Return to Answer

• 1

• 17.3k
• 4
• 31
• 89

A few things to look into

• This is a very bad idea in a header file:

using std::strlen;
using std::allocator;

Don't tamper with the global namespace. This exposes those names to every other file including the Str header, which in the long run is a recipe for name collisions. Better to just std:: qualify your names as needed. If you must using or using namespace, then do it only in the implementation file (.cpp).

As a side note, consider always namespacing your stuff. Str is a very common name used by many codebases. If you leave it as a global name, it can become a conflict if you end up merging your code with some other third-party codebase. Namespaces are a great underused feature of C++.

• I would prefer if your operators where non-members and non-friends of the class. They are already non-members, but making then friends is just as good, they can mess around with the internal data. Prefer defining a minimal public interface that the operators can build on. For instance, operator + could be defined in terms of +=:

   Str operator + (Str lhs, const Str& rhs) {
   lhs += rhs;
   return lhs;
   }
  

• Consider adding move support. Since your class is just a pointer wrapper, it can easily be moved. This could turn out to be a big performance boost with very little implementation cost.

• Speaking of move and looking at the assignment operator, I would like to see the use of copy-and-swap style in there. It is a well stablished way of handling copy in these kinds of resource containers. I strongly suggest you take a look.

• The copy constructor is missing entirely though, which is a major design flaw. If you did Str a(b);, where b is another Str, you will just get a shallow pointer copy, but the destructors won't know that and both will attempt to free the same pointer. You must fix this before your code is anywhere near usable.

• Default constructor is broken. It doesn't initialize the member data to safe defaults, so you effectively get garbage data in the pointer and size. Fix it by explicitly setting the pointer to nullptr and the size to zero.

• It's a bit pointless resetting the data in the destructor. The object is gone after that.

~Str() {
 if (data) alloc.deallocate(data, length);
 data = 0; // <-- use nullptr!
 length = 0;
 }

Another little issue there: use nullptr for pointers. 0 is implicitly convertible to pointer, yes, but zero evokes numbers. Not quite the same thing!

• begin/end need const overloads, also retuning a const char* (but wait, shouldn't they return iterators instead?).

• c_str() allocates memory... Wait whaaaa??? Very unexpected behavior there for a method that mimics std::string::c_str(). It should just be a lightweight accessor that returns a read-only pointer to the underlaying null-terminated string. I can't tell 100% by reading the code if data is always '0円' terminated, but if it is not, then it should be. It only costs you an extra byte to ensure the underlaying buffer is always a valid C-string, so do that and avoid a large series of problems.

• Same would apply to rawdata(), even though I really don't see much of a reason for its existence in the first place.

A couple departing notes on optimization

SSO:

The first obvious optimization here would be to add Small String Optimization (SSO). The idea behind it is that a lot of times the strings are small. So we might benefit from having a local inline char buffer within the string object itself to prevent an expensive dynamic allocation for the short string case. In terms of code, it might look a bit like this:

class Str
{
 char* data;
 char smallstr[N];
};

Where N is a carefully chosen constant to balance between memory usage and average size of strings. Then data will either point to the local inline buffer if the string fits in it, or to heap-allocated memory if it doesn't.

EBO:

Empty Baseclass Optimization (EBO) could be applied to your use of std::allocator. The default Standard allocator is a stateless class. It just forwards every call to operator new and delete. Problem is, if you declare an instance of allocator, it still takes some space. This happens because of a few requirements of the C++ Standard that I will not go in detail here (check the link above for full info). Suffice to say, you are paying for some extra storage space that is not needed by the allocator itself.

So to prevent std::allocator from taking any space in our class, rather than declaring an instance of it, you could take advance of private inheritance and leverage EBO. If you inherit from an empty class, then it is guaranteed to take no extra space in the resulting child type:

class Str : private std::allocator<char>
{
 // as before
};

Private inheritance won't expose any of the allocator's interface publicly in Str, which precisely what we want here.