3
\$\begingroup\$

For a very specific usecase, I need a special version of a string, which is not only zero-terminated, but also begins with '\0'. So, the string should look like this in memory: \0Test wefjwef\0.

For this, I programmed the following class:

class LeftZeroPaddedString: private std::vector<char>
{
public:
  explicit constexpr LeftZeroPaddedString(const char* _begin, const char* _end)
  : std::vector<char>((_end - _begin) + 2)
    {
      this->front() = '\0';

      if(_begin != _end)
        {
          std::copy(_begin, _end, std::vector<char>::begin() + 1);
        }
      
      this->back() = '\0';
    }

  explicit constexpr LeftZeroPaddedString(const std::string_view &_text)
  : LeftZeroPaddedString(_text.begin(), _text.end())
    { }

  explicit constexpr LeftZeroPaddedString(void)
  : LeftZeroPaddedString("")
    { }

  explicit constexpr LeftZeroPaddedString(const LeftZeroPaddedString& _other)
    { *this = _other; }

  /**
  * Returns the size of the underlying vector.
  *
  * @return the size of the underlying vector, includes the null terminator at the end and at the front
  */
  constexpr size_t byteSize(void) const
  { return std::vector<char>::size(); }

  /**
   * @return the size of the text part, without the null terminator at the front and at the back
   */
  constexpr size_t size(void) const
  { return this->byteSize() - 2; }

  /**
   * @return true if the string is empty and contains only the front-facing and the back-facing null terminator
   */
  constexpr bool empty(void) const
  { return this->byteSize() == 2; }


  /**
  *     \0Hello World!\0
  *                    ⬆️
  * @return a pointer to the second zero terminator in the String
  */
  constexpr char *bytesBegin(void)
    {
      assert(std::vector<char>::operator[](0) == '\0');
      return &std::vector<char>::operator[](0);
    }

  constexpr const char *bytesBegin(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->bytesBegin(); }

  /**
  *     \0Hello World!\0
  *      ⬆️
  * @return a pointer to the first zero terminator in the String
  */
  constexpr char *bytesEnd(void)
    {
      assert(std::vector<char>::operator[](std::vector<char>::size() - 1) == '\0');
      return &std::vector<char>::operator[](std::vector<char>::size() - 1);
    }

  constexpr const char *bytesEnd(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->bytesEnd(); }

  /**
  *     \0Hello World!\0
  *       ⬆️ (to the 'H')
  * @return a pointer to the first character in the String
  */
  constexpr char *textBegin(void)
    {
      if(this->empty())
        { return this->bytesBegin(); }
      else
        {
          return this->bytesBegin() + 1;
        }
    } 

  constexpr const char *textBegin(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->textBegin(); }

  /**
  *     \0Hello World!\0
  *                  ⬆️ (to the '!')
  * @return a pointer to the last character in the String
  */
  constexpr char *textEnd(void)
    {
      if(this->empty())
        { return this->bytesBegin(); }
      else
        {
          return this->bytesEnd() - 1;
        }
    }

  constexpr const char *textEnd(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->textEnd(); }

  constexpr char& operator[](const size_t& _i)
  { return *(this->bytesBegin() + 1 + _i); }

  constexpr const char& operator[](const size_t& _i) const
  { return static_cast<const std::vector<char>*>(this)->operator[](_i + 1); }

  constexpr char& at(const size_t& _i)
  { return static_cast<std::vector<char>*>(this)->at(_i + 1); }

  constexpr const char& at(const size_t& _i) const
  { return static_cast<const std::vector<char>*>(this)->at(_i + 1); }

  void print(void) const
    {
      for(const char& _c: *static_cast<const std::vector<char>*>(this))
        {
          std::cout << "" << _c;
        }
    }

  constexpr auto begin(void)
  { return ++std::vector<char>::begin(); }

  constexpr auto begin(void) const
  { return ++std::vector<char>::begin(); }

  constexpr auto cbegin(void) const
  { return ++std::vector<char>::cbegin(); }

  constexpr auto end(void)
  { return --std::vector<char>::end(); }

  constexpr auto end(void) const
  { return --std::vector<char>::end(); }

  constexpr auto cend(void) const
  { return --std::vector<char>::cend(); }

  constexpr auto rbegin(void)
  { return ++std::vector<char>::rbegin(); }

  constexpr auto rbegin(void) const
  { return ++std::vector<char>::rbegin(); }

  constexpr auto rcbegin(void) const
  { return ++std::vector<char>::crbegin(); }

  constexpr auto rend(void)
  { return --std::vector<char>::rend(); }

  constexpr auto rend(void) const
  { return --std::vector<char>::rend(); }

  constexpr auto rcend(void) const
  { return --std::vector<char>::crend(); }

  constexpr const char* c_str(void) const
  { return this->textBegin(); }

  constexpr std::string_view cppStringView(void) const
  {
    if(this->empty())
      { return std::string_view(""); }
    else
      { return std::string_view(this->textBegin(), this->bytesEnd()); }
  }

  constexpr std::string cppString(void) const
  { return std::string(this->cppStringView()); }

  constexpr bool operator==(const std::string_view& _sv) const
  { return this->cppStringView() == _sv; }

  constexpr bool operator==(const LeftZeroPaddedString& _other) const
  { return this->operator==(_other.cppStringView()); }

  constexpr LeftZeroPaddedString& operator=(const LeftZeroPaddedString& lzpd)
  {
    const std::vector<char>* lzpd_super = &lzpd;
    std::vector<char>::assign(lzpd_super->begin(), lzpd_super->end());
    return *this;
  }

  constexpr void clear(void)
  { *static_cast<std::vector<char>*>(this) = {'\0', '\0'}; }

  constexpr void swap(LeftZeroPaddedString& _right)
  {
    const LeftZeroPaddedString buffer(_right);
    _right = *this;
    *this = buffer;
  }
};

Update: Thanks for the good reviews, I want to briefly explain my use-case. I need this special form of a string for a very specific parser, that does not iterate the characters towards the end, but also to the the front. Thus, I need to know when the char* pointer has reached the front, in order for the parser to know when it should cease to decrease the pointer.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I’m going to write up a proper review for this code, but in the meantime, I should point out that you could use std::string for this. std::string can hold NUL characters in the middle of the string. It’s just tricky to create such strings (because you generally can’t do it from string literals directly). For example \$\endgroup\$
    – indi
    Commented Jul 12 at 20:43

2 Answers 2

7
\$\begingroup\$

Design review

I mentioned in a comment that you could use a std::string for this. However, that would be a quick-and-dirty solution. When you have a requirement that has to be true, making a custom type is exactly the right solution. Once you have a working type that enforces the constraints you need, everything else is trivial, and safe by default.

What is the point?

You have neglected to explain why you need this particular abstraction, instead offering only vagaries like “a very specific use case”. Because of that, you can’t possibly get a good code review. Without knowing what the problem is, no-one can possibly tell you whether your solution is good, or completely idiotic.

Even just trying to consider the idea in the abstract is difficult, because to know whether an interface is “good” or “useful”, you have to know what a “normal” usage would look like. Like, does it make sense to silently “hide” the prefix and postfix NUL characters in functions like .at() or .empty()… but not hide them in .print()?

Here’s the bottom line: You have claimed you “need” a string with NULs at either end… but you have made a string that hides the NULs at either end. So… huh? 🤷🏼 Do you need the NULs or not? And if you really do need the NULs, why does your interface work so hard to make them disappear?

Another thing that makes me scratch my head is the distinction between “text” and “bytes”. It seems like you intend for the type to “look like” a string through the “text” view, but provide access to the NUL-capped data through the “bytes” view, which is fine… except then you have a bunch of functions that make the “text” view the “true” view (like .size()). So these are not two different ways to view the same data, but rather one true/correct view (as “text”), and one sort of second-class citizen view (that is, ironically, the actual truth of the data).

So when I ask “what is the point?”, I am not implying the type is useless, but rather that you are confused about what it will be used for. Is it a string? Or is it a container of character data that is guaranteed to be NUL terminated at both ends?

The name

@J_H already mentioned the problem with “zero”. A “left zero-padded string” already means something: "00000foo". (That would be “foo” left zero-padded to a width of eight.) The correct name for the character is “NUL”. Some people also use “NULL”, but be careful, because that can also mean the null pointer.

So what you want is not LeftZeroPaddedString, it is LeftNulPaddedString.

Except… that’s still not correct.

Because “padding” is not what you are doing. When I see the name LeftZeroPaddedString (or LeftNulPaddedString), I expect that the type allows me to specify a string and a width, and if the string length is less than the width, it fills up the “left” side with extra zeros (or NULs) to make up the space.

I’ll also point out that the “left” part is dodgy, too. Historically, people just assumed all text was English… but this is the 21st century. The beginning of a string is not necessarily the left part of the string. That depends on what the language is. Some languages are right-to-left. Others are top-to-bottom! (There are even a very small number of bottom-to-top languages.)

So I would suggest what you are making is not a “left zero-padded string”, but rather a “NUL-prefixed string”. “Prefix” is valid for any language direction.

Or maybe it’s not a string at all. Maybe it’s a “NUL-bordered character array”.

Figuring out exactly what it is is an important first step is getting the abstraction right.

vector is a poor choice

You have based your type on std::vector<char>. I assume that is because you were not aware you could put NUL anywhere in a std::string. But you can!

And because you can use std::string, you should use it. It is better than std::vector, if only for the fact that the small-string optimization is possible. Because of the small-string optimization, you can assume that, generally, any string of less than 15 characters will not require any dynamic memory allocation; it can all be stored very efficiently within the string itself. And short strings are very common. In fact, your example string—"Test wefjwef"—even with the extra NUL, would fit.

However, because you have used std::vector, even an “empty” string requires dynamic allocation. That is expensive, and dangerous, because users often count on noexcept default construction for strings… but you can’t offer that.

If std::vector supported a “small-string optimization”, then I would enthusiastically recommend it over std::string, because I think this type should be an abstraction of “container of bytes”, not “sorta-kinda string”.

Inheritance is a poor choice

There is really no sensible reason to inherit from std::vector (or std::string), rather than to include one as a data member. In practice, you have to override or re-implement virtually every function anyway, so there isn’t even any code reuse benefit.

There is a general rule: prefer composition to inheritance. Technically this doesn’t really apply here, because you are using private inheritance. But still, using data members is just the more “natural” and “clear” way to to write classes.

We’ll see more reasons why inheritance is bad design later on.

The interface

You are making a string, basically. Thus, it should have a string-like interface.

Now, there are actually two options for this. One option is just to look at the interface for std::string, and duplicate it. The other option is… not: Make whatever interface you like for your type, but provide a conversion to std::string_view.

I think the best option in your case is to not duplicate the std::string interface, because your class is… weird in a lot of ways.

Instead, I think you should just create a container-like interface that does not hide the leading and trailing NULs… and provide conversions to std::string and std::string_view.

Put another way: When I asked whether the type is supposed to be a string or a character container that is NUL terminated at both ends… I am suggesting the answer should be the latter. Don’t pretend this is a simple string. Don’t fake being a string by lying in functions like .size() and .empty(). Accept what this is: a special container for string data that NUL terminates both ends. It is not a string itself, but it can provide string views of the data between the NULs.

(Then, the next thing to consider is whether it should be valid for an “empty” object to have only a single NUL, or if it must have two. Again, no-one can guess what the correct answer to this is without knowing more about your use case.)

I could tell you that your abstraction is a mess, but I think the most effective way to prove the point is with an illustration. What should be the output of the following program?

auto main() -> int
{
    using namespace std::literals;

    auto const v = LeftZeroPaddedString{"foobar"sv};
    std::println("size                           = {}", v.size());
    std::println("distance(begin, end)           = {}", std::ranges::distance(v.begin(), v.end()));
    std::println("distance(bytesBegin, bytesEnd) = {}", std::ranges::distance(v.bytesBegin(), v.bytesEnd()));
    std::println("distance(textBegin, textEnd)   = {}", std::ranges::distance(v.textBegin(), v.textEnd()));
}

Here’s what I got:

size                           = 6
distance(begin, end)           = 6
distance(bytesBegin, bytesEnd) = 7
distance(textBegin, textEnd)   = 5

Now, I’ll accept that the “text” distance should be two less than the “bytes” distance, because the “bytes” distance includes the two NULs. But how is it that neither the “text” distance nor the “bytes” distance is the same as the size?!

Clearly something has gone wrong.

char/byte confusion

A char is not a byte. They are defined to be the same size, yes, but they are distinct types, and totally different abstractions that represent different concepts.

See for yourself.

Yes, a char can be converted to a byte, and vice versa, without loss of information. But they are not the same.

As I said, I think you should make this abstraction a container of bytes, and provide string views as an extra interface. So this shouldn’t be based on std::vector<char>, it should be based on std::vector<std::byte>.

What about allocators?

So, I assume you have no interest in supporting other character types (because that would completely break the whole text/bytes dichotomy). I also assume you have no interest in character traits (because, again, I don’t think you really mean for this type to be a string abstraction, I think you mean for it to be a byte container, that has some string view capability).

However, why not support allocators?

After all, you get allocator support more or less for free, whether you use std::vector or std::string as the underlying type.

You could transform the type to a template, and add an allocator template parameter. Or you could avoid having to make the class a template by using polymorphic allocators.

Either way, you should consider allocator support.

You need to test your code

One of the most important skills you can learn as a programmer—not just for C++ but for any language—is testing.

I can tell you did not test your code. I can tell because—not only does it have some nasty and rather obvious bugs that would have been trivially found with even a basic testing setup—I can tell the code was not tested because its design is broken.

Here’s the thing with testing: it doesn’t just help you find bugs and broken code. If you write your tests first, it forces you to think about the design, and make it useful and sensible from the start.

I pointed out above that this type presents at least three different sizes: the .size() size, the “text” range size, and the “bytes” range size. That’s nonsense, and there is no way anyone would have made a class that does that by design. The only way this could happen is by just blindly slapping together a class, without proper forethought and planning.

TEST YOUR CODE. And by that, I don’t mean “slap together a simple program with some asserts, or just print some stuff then read it to see if it’s right”. I mean use a proper testing framework, and—and this is very important—write the tests first, before you write the class. Think about your expectations for how things should work, write the tests to check for those expectations, then write the code to pass the tests. That is the path to good code.

Code review

  explicit constexpr LeftZeroPaddedString(const char* _begin, const char* _end)
  : std::vector<char>((_end - _begin) + 2)
    {
      this->front() = '\0';

      if(_begin != _end)
        {
          std::copy(_begin, _end, std::vector<char>::begin() + 1);
        }
      
      this->back() = '\0';
    }

  explicit constexpr LeftZeroPaddedString(const std::string_view &_text)
  : LeftZeroPaddedString(_text.begin(), _text.end())
    { }

  explicit constexpr LeftZeroPaddedString(void)
  : LeftZeroPaddedString("")
    { }

So these are your three constructors. You have chosen to use the first one—the one that takes an iterator pair (technically, a pair of pointers)—as the “base” constructor, and make every other constructor delegate to that.

Here’s the problem with that: that iterator pair constructor is a rather “expensive”, and “risky” constructor, because it needs to allocate memory, and because it might fail. Also, if it were written well, it would have to do quite a significant number of checks. For example, it needs to check that both pointers are not nullptr, and that they are a valid range. If any of those checks fail, again, the constructor might fail.

All of this is signalling that this is not the right way to do this.

Let’s go in reverse order and start with the default constructor. Now, the default constructor is a critical part of a class. It’s quite peculiar to have it delegate to anything else. But even if it does, it should still be noexcept, if at all possible. With vector as the base, that is not possible… but it is possible with string as the base (so long as you allow a single NUL in the empty case… if not, then you would have to implement everything manually, which, honestly, would not be all that hard).

But the important thing is: implementing the default constructor in terms of other public constructors is a bad idea, because other public constructors will have to do checks on the input data that will just be a waste of time for the default constructor (because you control the input data in that case).

You can implement the default constructor in terms of other constructors, but they should either have a wide contract or be private. They must be constructors that have no need to do any checks on the arguments, either because all arguments are valid (so checking is unnecessary) or you are sure it can never be called with invalid arguments (because it is private).

So, for example, with a private constructor as your basis constructor:

struct internal_t {};

class LeftZeroPaddedString
{
private:
    constexpr explicit LeftZeroPaddedString(internal_t, std::string_view sv)
    {
        _data.resize(2 + sv.size());
        std::ranges::copy(sv, _data.data() + 1);
        _data.front() = '\0';
        _data.back() = '\0';
    }

    static constexpr auto _validate(char const* first, char const* last)
    {
        if (first == nullptr)
            throw std::invalid_argument{"first is nullptr"};
        if (last == nullptr)
            throw std::invalid_argument{"last is nullptr"};

        // other checks...

        return std::string_view{first, last};
    }

    static constexpr auto _validate(std::string_view sv)
    {
        // any checks you want...

        return sv;
    }

    std::vector<char> _data;

public:
    constexpr explicit LeftZeroPaddedString()
        : LeftZeroPaddedString{internal, std::string_view{}}
    {}

    constexpr explicit LeftZeroPaddedString(char const* first, char const* last)
        : LeftZeroPaddedString{internal, _validate(first, last)}
    {}

    constexpr explicit LeftZeroPaddedString(std::string_view text)
        : LeftZeroPaddedString{internal, _validate(text)}
    {}

But let’s get back to the code as actually written:

  explicit constexpr LeftZeroPaddedString(void)
  : LeftZeroPaddedString("")
    { }

First, we don’t put void in empty argument lists. That is an archaic C practice.

Second, there doesn’t really seem to be a reason to make this constructor explicit. That just makes using this type more verbose, for no good reason.

Third, if you are making an empty std::string_view, using "" is a bad idea. When you do "", you are calling the string view constructor that takes a pointer to a NUL-terminated character string. That constructor has to be quite a bit of work. It has to do a bunch of checks (is the pointer nullptr?), and then it has to de-reference the pointer, and do a string length check. With luck, the compiler can optimize all this away… but why not just say what you mean instead? You don’t want a string view of "". You want an empty string view. Don’t get there the hard way. Be explicit and direct: std::string_view{}.

So:

    constexpr LeftZeroPaddedString()
        : LeftZeroPaddedString{std::string_view{}}
    {}

Now, as for the other two constructors, it is technically irrelevant whether you implement one in terms of the other, or vice versa. However, I would always recommend using the “safer” alternative as the basis operation. If the “safer” alternative is the basis, then when you use it directly, you don’t need to do the extra checks required for the “less-safe” alternative. But if the “less-safe” alternative is the basis, you always pay for those checks, even when unneeded.

In this case (and in general), the range version is safer, because there is no chance of the iterators being from different ranges, or mixing up the begin/end iterator, etc.. So the basis should be string view… not the iterator pair.

  explicit constexpr LeftZeroPaddedString(const std::string_view &_text)
  : LeftZeroPaddedString(_text.begin(), _text.end())
    { }

First you should never take view types by reference. They are meant to be taken by value.

Second, in C++, we keep type information together. That means:

  • const std::string_view &_text: This is C-style.
  • const std::string_view& _text: This is C++-style.
  • std::string_view const& _text: Also C++-style.

Third, if you are going to use an underscore prefix, the place to use it would be for private data members… not function arguments. Function arguments take priority, so there is no sensible reason to mangle them.

Fourth, and this is the most important thing (and another reason why you should make the string view constructor the basis constructor)… std::string_view::begin() does not return a char pointer (const or otherwise). It returns a std::string_view::iterator. Now, std::string_view::iterator could be a char pointer (and I think it is in both libstdc++ and libc++)… but it does not have to be.

So what you would have to do is something more like (_text.data(), _text.data() + _text.size())… except that’s dodgy, because _text.data() might be nullptr, which you shouldn’t be adding anything to (including zero).

There are two options here:

  1. Do it the other way around. Implement the char pointer version in terms of the string view version.
  2. Change the char pointer version to a general iterator version.

Option 1 I’ve illustrated above.

Option 2 would look something like:

    template <std::input_iterator I, std::sentinel_for<I> S>
    requires std::indirectly_copyable<I, char*>
    explicit constexpr LeftZeroPaddedString(I first, S last)
    {
        if constexpr (std::forward_iterator<I>)
        {
            _data.resize(std::ranges::distance(first, last));

            auto p = _data.data();
            *p++ = '\0';
            *(std::ranges::copy(sv, p).out) = '\0';
        }
        else
        {
            _data.push_back('\0');
            std::ranges::copy(first, last, std::back_inserter(_data);
            _data.push_back('\0');
        }
    }

    explicit constexpr LeftZeroPaddedString(std::string_view text)
        : LeftZeroPaddedString(text.begin(), text.end())
    {}

And the last constructor:

  explicit constexpr LeftZeroPaddedString(const char* _begin, const char* _end)
  : std::vector<char>((_end - _begin) + 2)
    {
      this->front() = '\0';

      if(_begin != _end)
        {
          std::copy(_begin, _end, std::vector<char>::begin() + 1);
        }
      
      this->back() = '\0';
    }

This is where your idea of inheriting from vector really begins to stink (though it gets much, much worse later).

Firstly, I should note that we don’t write this-> in front of every member function or data member. It is very noisy.

But the real ugliness is here: std::vector<char>::begin().

Now, you know why you have to write this. It is because you have overridden begin() with a whole new implementation… one that skips that initial NUL. In order to get back the original vector implementation, you need that ugly and verbose incantation.

But here’s a damning question: What happens if, at some point in the distant future, you or someone else decides to also override front(), and have it return a reference to the first character after the initial NUL?

Oops.

Now, you can “guard” against this by uglifying every single vector call the same way you did for begin() here, and get this:

    constexpr explicit LeftZeroPaddedString(char const* first, char const* last)
        : std::vector<char>(std::ranges::distance(first, last) + 2)
    {
        std::vector<char>::front() = '\0';
        std::copy(first, last, std::vector<char>::begin() + 1);
        std::vector<char>::back() = '\0';
    }

And of course, you can “guard” every single case where you call a vector function and want the vector implementation.

So… problem solved?

No.

Look at this function:

  constexpr bool empty(void) const
  { return this->byteSize() == 2; }

“What’s the problem?”, you ask. “std::vector doesn’t have a .byteSize() function!”

You are correct…

… at least, up to C++23. And C++26 so far.

But std::vector might get a .byteSize() function in the future… and if/when that happens… boom.

And this is why implementing via inheritance is such a terrible idea. When you just include a vector in your type, you are insulated from any changes to vector. So long as the interface you use remains consistent, the standard can do whatever it pleases to vector; it can add new public or protected data members or member functions (or even static stuff), and you won’t be affected. But when you inherit from vector… you are a vector (even if privately), so any changes to vector change you. So ever time the standard gets updated, you will need to check that it hasn’t broken your class.

So, in summary:

  • Don’t implement your default constructor in terms of other constructors, unless they are no-fail or private.
  • Don’t implement range constructors in terms of iterator constructors. Ranges are always better than iterator pairs.
  • std::string_view::iterator is not necessarily a char const*.
  • Don’t inherit from stuff you don’t have to. Prefer composition to inheritance.

Moving on.

  explicit constexpr LeftZeroPaddedString(const LeftZeroPaddedString& _other)
    { *this = _other; }

Don’t write special member functions if you don’t have to.

You could just do this:

    // Note the `explicit` is utterly pointless here.
    constexpr LeftZeroPaddedString(LeftZeroPaddedString const&) = default;

But even that is not as good as:

    // ಠ_ಠ

Yes, that’s right: nothing.

The implicitly-defined default operations are just fine. So leave them implicitly defined.

In fact, by defining this constructor (and the move ops later), you have disabled move construction and move assignment. You have crippled the efficiency of the type… for nothing.

So just… don’t do it.

(Also, as an aside: You are implementing the copy constructor of a derived type, but for some reason, you don’t call the copy constructor of the base type (and, instead, you call the default constructor, then do copy assignment). That’s unwise.)

  constexpr size_t byteSize(void) const
  { return std::vector<char>::size(); }

  constexpr size_t size(void) const
  { return this->byteSize() - 2; }

  constexpr bool empty(void) const
  { return this->byteSize() == 2; }

So you have a “size” which is not actually the size of the data, and a function called “empty” that returns true if the size is 2.

Clearly something is wrong with this abstraction.

As I’ve suggested, don’t twist yourself in knots trying to create a string view interface that lies. Create an honest interface, and provide a string view conversion that gives you a view of the “text” data.

So, drop .byteSize(), and all of the other .bytes*() functions (like .bytesBegin(). Just let .size() return the truth: how many characters are in the container… including the NULs. And let all the other container functions also return truth: .begin() points to the first NUL, and .end() points to one past the last NUL.

Some other comments:

  1. There is no size_t. It is std::size_t, and you need to include the correct header. (It “works” for historical reasons, but that will change in the future.)
  2. These functions cannot possibly fail, under any circumstances. So they should be noexcept.

So:

    constexpr auto size() const noexcept { return _data.size(); }
    constexpr auto empty() const noexcept { return _data.empty(); }

Much simpler, eh?

  constexpr char *bytesBegin(void)
    {
      assert(std::vector<char>::operator[](0) == '\0');
      return &std::vector<char>::operator[](0);
    }

  constexpr const char *bytesBegin(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->bytesBegin(); }

Implementing the otherwise-identical const and non-const versions of a function using const_cast is an acceptable hack, but it would be wiser to implement the non-const version in terms of the const one.

The way you are doing it now, you are removing a const from something that comes with it. Is that safe?

On the other hand, if you did it the other way around:

    constexpr auto bytesBegin()
    {
        return const_cast<char*>(std::as_const(*this).bytesBegin());
    }

    constexpr auto bytesBegin() const
    {
        // implementation
    }

Now you are adding the const to something… and then removing it from the result. And you know this is safe because you know this was not const to begin with.

That is the original way Meyers (I believe) formulated it.

Of course, there is a better solution these days:

    template <typename Self>
    constexpr auto bytesBegin(this Self&& self) const
    {
        return std::forward<Self>(self).data();
    }

But that’s another story.

  constexpr char *bytesEnd(void)
    {
      assert(std::vector<char>::operator[](std::vector<char>::size() - 1) == '\0');
      return &std::vector<char>::operator[](std::vector<char>::size() - 1);
    }

What… what?!

Are you trying to confuse people and create bugs.

The universal, standardized definition of “begin” and “end” in C++ is a half-open range, where “begin” points to the first item in the range, and “end” points to one step past the last item.

You have defined .bytesBegin() to point to the first item (the initial NUL). That’s fine.

But then you define .bytesEnd() to point to the last item (the final NUL)… not the one past the last item!!!

That’s absurd. Did you try to use the type? Did you even test it? Everything will break. Nothing will work as you expect.

In fact, you even wrote your string view conversion like this:

std::string_view(this->textBegin(), this->bytesEnd());

The hell? Did you not see using a different types of iterators in a single operation like this as a problem? If you’d actually implemented real iterator types, rather than just using pointers, this would not even have compiled. (This is why it is a good idea to use real iterator types, at least when debugging, even if raw pointers would work.)

We’ll be coming back to this mess later.

  constexpr char *textBegin(void)
    {
      if(this->empty())
        { return this->bytesBegin(); }
      else
        {
          return this->bytesBegin() + 1;
        }
    } 

  constexpr const char *textBegin(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->textBegin(); }

  constexpr char *textEnd(void)
    {
      if(this->empty())
        { return this->bytesBegin(); }
      else
        {
          return this->bytesEnd() - 1;
        }
    }

  constexpr const char *textEnd(void) const
    { return (const_cast<LeftZeroPaddedString*>(this))->textEnd(); }

I think you’ve kinda lost the plot here (which is another sign that your abstraction is a mess).

When .empty() is true, there are 2 NULs in the data. Always always. .empty() is literally defined as “size equals two”. .bytesBegin() always points to the initial NUL. .bytesEnd() always points to the trailing NUL. You have asserted these facts (misguided though they may be, in the latter case).

So… what is the point of doing the checks and branching. By definition…:

    constexpr auto textBegin() { return ++bytesBegin(); } 

    constexpr auto textEnd() { return --bytesEnd(); }

must be correct.

And they cannot even conceivably fail, so you could even make them noexcept.

  void print(void) const
    {
      for(const char& _c: *static_cast<const std::vector<char>*>(this))
        {
          std::cout << "" << _c;
        }
    }

I’m going to assume this is a debug function you just forgot to remove. Because it prints the NULs… which don’t really show up… and there is an unnecessary "" there.

But if this is intended to be a real function then…

  1. What is the point of making _c a const char& instead of just a char?
  2. Rather than hard-coding std::cout, you should take a std::ostream& parameter.
  constexpr std::string_view cppStringView(void) const
  {
    if(this->empty())
      { return std::string_view(""); }
    else
      { return std::string_view(this->textBegin(), this->bytesEnd()); }
  }

cppStringView() is kind of an ugly name. What other kind of string view could you be expecting, if not a C++ string view? A Python string view?

Rather than a function, this would be better as a conversion operator:

constexpr operator std::string_view() const& noexcept
{
    return {textBegin(), textEnd()};
}

Note the lvalue reference specifier. This is to prevent creating views to temporaries. (And yes, I know this implementation is incorrect, but… that’s the next issue.)

Now, I pointed out the absurdity of mixing iterator types above, but here is where the rubber hits the road. The fact that you have to mix the iterator types here to make this work is a screaming warning alert that something is very, very wrong.

But don’t take my word for it. Try this program:

auto main() -> int
{
    using namespace std::literals;

    auto const s = "foobar"s;
    std::println("initial string     = {}", s);

    auto const v = LeftZeroPaddedString{s};
    std::println("via string view    = {}", v.cppStringView());
    std::println("via text begin/end = {}", std::string{v.textBegin(), v.textEnd()});
}

Look what I get:

initial string     = foobar
via string view    = foobar
via text begin/end = fooba

Notice anything wrong?

  constexpr bool operator==(const std::string_view& _sv) const
  { return this->cppStringView() == _sv; }

  constexpr bool operator==(const LeftZeroPaddedString& _other) const
  { return this->operator==(_other.cppStringView()); }

If you are going to include conversions, you should probably go all the way and define operator<=>.

  constexpr LeftZeroPaddedString& operator=(const LeftZeroPaddedString& lzpd)
  {
    const std::vector<char>* lzpd_super = &lzpd;
    std::vector<char>::assign(lzpd_super->begin(), lzpd_super->end());
    return *this;
  }

This operator is not necessary. The default is fine.

  constexpr void swap(LeftZeroPaddedString& _right)
  {
    const LeftZeroPaddedString buffer(_right);
    _right = *this;
    *this = buffer;
  }

Do you realize this swap operation is copying the string every time? Like, three copies, just to do a swap?

Do you not know about moving? This should be:

    constexpr auto swap(LeftZeroPaddedString& right)
    {
        auto buffer = std::move(right);
        right = std::move(*this);
        *this = std::move(buffer);
    }

Or, even better:

    constexpr auto swap(LeftZeroPaddedString& right) noexcept
    {
        std::ranges::swap(
            static_cast<std::vector<char>&>(*this),
            static_cast<std::vector<char>&>(right)
        );
    }

Note that we can now make this noexcept, because vector moves are noexcept.

Or, even more better, by including the vector as a data member rather than inheriting:

    constexpr auto swap(LeftZeroPaddedString& right) noexcept
    {
        std::ranges::swap(_data, right._data);
    }

In summary:

  • Inheriting from std::vector is a terrible idea. There are literally no benefits over including a vector as a data members, and the costs are that your code is damn near illegible due to all the casting and explicit function calls, and your type is brittle and can silently break if/when vector adds more member functions in future standards.

  • Your abstraction is poorly conceived, and nonsensical. Sometimes your type represents just the stuff between the two NULs. Sometimes it includes the two NULs. Sometimes it only includes one of them. This confusion has created actual bugs.

  • Less code is better. For starters, having to constantly qualify every function call with std::vector<char>:: or casting to std::vector<char> in one form or another is a sign that you’re doing something wrong. And you should default everything that can be defaulted, and not write any special operations that the implicit defaults work for.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I’ve learned a lot about C++ from reading your reviews over the last years. I didn’t think this question would teach me anything, but it still gave you the opportunity to point out some details I wasn’t aware of. Thank you! \$\endgroup\$ Commented Jul 14 at 16:11
4
\$\begingroup\$

names

The code works. The names are unclear.

When I first read this, I believed that “zero“ was “0”, ASCII 48. Please refer to it as NUL instead. Comments should refrain from mentioning "null terminator", as null is a memory address pointer, such as what malloc() returns when there's no more room.

Your initial NUL is more of a Delimiter than a Terminator.

swapped comments

Saying the code works is different from saying there's no defects.

  /**
  *     \0Hello World!\0
  *                    ⬆️
  * @return a pointer to the second zero terminator in the String
  */
  ...
  /**
  *     \0Hello World!\0
  *      ⬆️
  * @return a pointer to the first zero terminator in the String
  */

Anyone reading this will conclude that appropriate method names are bytes{End,Begin}. Yet in the OP code they are associated with bytes{Begin,End}. Yes, sometimes comments lie.

I do like the assert(); thank you.

extra check

The _begin != _end test seems a premature optimization, and is unlikely to be a common enough use case to be worth it. We can std::copy() a count of zero bytes just fine.

unicode

Consider documenting whether these strings are intended to be exclusively 7-bit US ASCII, or whether valid UTF-8 is also part of your supported use case.

internal NUL

There's no checks for someone trying to create a padded string with input like "ab\0cd". And print(), instead of being implemented in the natural way, goes to some trouble to iterate over bytes. This seems to suggest that you support storing invalid UTF-8 and also internal NULs (binary data). It would be worth clarifying the rules of the Public API you're designing, and maybe enforcing the rules more strictly.

\$\endgroup\$
5
  • 3
    \$\begingroup\$ Comments should refrain from mentioning "null terminator": A "null-terminated string" is a perfectly cromulent way of referring to C-style strings. "Null terminator" or "null character" are common expressions. \$\endgroup\$
    – isanae
    Commented Jul 12 at 19:10
  • \$\begingroup\$ I do like the assert(); thank you. I don't. Compile that code with NDEBUG defined and the assert disappears: "If NDEBUG is defined as a macro name at the point in the source code where <cassert> or <assert.h> is included, the assertion is disabled: assert does nothing." Nevermind what appears to be a general-purpose library function blowing up the entire program on incorrect input seems like an extreme overreaction worthy of Boeing's 737MAX flight control software: "Hey! We're not sure what's going on, so let's just crash!" \$\endgroup\$ Commented Jul 12 at 21:44
  • 1
    \$\begingroup\$ @AndrewHenle, what are you upset with, pick one? (A.) The release engineer compiled and shipped with NDEBUG defined. (B.) ... with NDEBUG undefined. (C.) The code is responsible for maintaining more than one invariant. (D.) The developer expressed a belief about invariants via code rather than via documentation. Also, I don't understand your remark about "incorrect input". The pair of assert()s won't check what the calling app does. They're all about this library maintaining its own invariants. We are sure what's going on -- the invariants hold. We believe that; they are frequently verified. \$\endgroup\$
    – J_H
    Commented Jul 12 at 22:16
  • \$\begingroup\$ assert() either ignores the error or aborts the entire process. Neither is appropriate for a library function - it has no right to decide how the caller needs to respond to invalid input. Unless, like I said, you're the 737MAX flight control software where random crashes are seemingly the goal. Throwing lots of jargon around doesn't change that. \$\endgroup\$ Commented Jul 12 at 23:33
  • 2
    \$\begingroup\$ @AndrewHenle you're not listening. We signal an error if bad user input comes from calling app, perhaps via assignment to errno. We assert(some_invariant) for things that are true by construction. We believe the library code is correct. We believe an invariant is always true. During testing we want to know if some incorrect code crept in. An assert() helps with that. Now, some folks use assert() in situations where they should have thrown an exception, sure, that's a bad practice. But in the OP code, it is fundamentally impossible for caller to present an input leading to a violated invariant. \$\endgroup\$
    – J_H
    Commented Jul 13 at 0:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.