ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.25k stars 392 forks source link

Copy constructor of etl::string_ext erases the string? #963

Closed basprins closed 1 month ago

basprins commented 1 month ago

Hi,

I hope I am not making a really stupid mistake here. I use the etl::string_ext where I use my own external memory (in this case, allocated in FreeRTOS, but in principle that should be irrelevant).

I typically pass the etl::string_ext by reference, and then all goes fine. But this time, I passed it by value, because of no real reason, mostly "because I should be able to" since the etl::string_ext has a copy constructor

    //*************************************************************************
    /// Copy constructor.
    ///\param other The other string_ext.
    //*************************************************************************
    string_ext(const etl::string_ext& other, value_type* buffer, size_type buffer_size)
      : istring(buffer, buffer_size - 1U)
    {
      this->assign(other);
    }

But when I do this, the contents of the string is assigned with "other". The assign method calls into initialize which erases the memory, since the copy and the origin instance of etl::string_ext point to the same external memory (which in my case, is intentional).

I replaced the assign with a uninitialized_resize so that the copy has a correct size. And then it works fine.

Am I using the class in the wrong way? Or is this a bug in etl?

jwellbelove commented 1 month ago

Calling string_ext(const etl::string_ext& other, value_type* buffer, size_type buffer_size) a copy constructor is really a bit of a misnomer. It's not a real copy constructor, as it has additional arguments.

It's more like "Construct a new string_ext with a new buffer and copy the characters found in the other string_ext.

It is expected that the new buffer is different to the one being copied from.

I originally did not define a standard copy constructor as I thought it could result in bugs due to people thinking it was working the same as an etl::string copy constructor and creating a new separate copy of a string.

For string_ext it would have created two string_ext objects that point to the same buffer, which I think is what you are trying to do. I'm still not sure if this is a good idea or not.

You could use the existing string_ext(const char* text, char* buffer, size_type buffer_size) where, if text and buffer are the same address, no copy is made, just an update to the size.

basprins commented 1 month ago

Hi @jwellbelove Yeah I figured that. I extended the string_ext class and that class does have a true copy constructor. It calls the "copy constructor" of string_ext with the buffer of "the other" instance. I changed etl's copy constructor to allow this by checking if the buffers are pointing to the same address. It works fine.

But your comments make perfect sense too. It wasn't intended to be used this way. Fair enough. I'll stick with my small change, and hopefully remember that if I ever pull latest etl in my project :).

Thanks for the response though!