String: Fix "Use of memory after it is freed" when assigning to self

[asmaloney]

destruct() can call delete[], but then we use the memory in operator=.

Found using clang static analysis.

[asmaloney]

Looks like an AppVeyor timeout and I can't restart it...

[LB--]

Out of curiosity, is there some reason the assignment operator isn't using the copy and swap idiom? My understanding was that it was regarded as the better way to avoid this problem, as well as only needing to write one version of the operator. All the code is already in place for the move assign operator, the copy assign operator could be removed and the move assign operator could be changed to take the parameter by value instead of by rvalue reference.

[asmaloney]

@LB-- I'm not totally up on the move/rvalue reference semantics. If I'm following what you're suggesting we'd end up removing:

String& operator=(const String& other);
String& operator=(String&& other) noexcept;

// current implementation of:
String::String(String&& other) noexcept;

and replacing with this:

String::String(String&& other) noexcept
   : String()
{
   swap(*this, other);
}
                                                                                           
String& String::operator=(String other)
{
   swap(*this, other);

   return *this;
}

void swap(String& first, String& second)
{
   using std::swap;

   swap(first._large.data, second._large.data);
   swap(first._large.size, second._large.size);
   swap(first._large.deleter, second._large.deleter);
}

Is that about right?

[LB--]

@asmaloney The swap function isn't strictly necessary but yes, that is the general idea: re-use the code and exception guarantees of the copy constructor while also enabling compiler optimizations and move assignments, all in the same place by simply swapping. I was more asking @mosra though since I was curious if there was some blocker for using copy and swap here.

[mosra]

@asmaloney hi, congrats on having PR #100 :tada:

Hmm, this is one of those oneliner changes that require a lot of discussion :) So far I handle self-assignment in none of the classes, so this check either needs to be done everywhere or nowhere. My rationale was, until now, the following:

• Self-copy-assignment is a thing that the original C++ FAQ and many tutorials suggested to deal with. However, none of that is done for self-move-assignment and the C++ FAQ says "Self-assignment is not valid for move assignment." without any further explanation, which originally led me to believe this is something the C++ community rethought over time and nowadays it's not considered a problem anymore (in a similar spirit as checks for this == nullptr). Why should I care about copies but at the same time pretend that a = std::move(a) never happens?
• Not sure where I read that (can't find the source anymore), but guidelines of one larger C++ project encouraged to not bother and enable -Werror=self-assign instead, with a rationale that self-assign is an error anyway and as such should be caught at the source and not mitigated at the library level. In a similar manner, memcpy() also doesn't handle overlapping memory regions and requires the users to pay attention instead.

What was your use case that triggered a self-assign? In my experience, self-assign happened only very rarely, usually during hasty refactorings, and in most cases was caught by the compiler anyway. But I'm happy to be proven wrong / learn something new.

Out of curiosity, is there some reason the assignment operator isn't using the copy and swap idiom?

@LB-- as crazy as it might sound, frankly it's because this is the first time I hear about it :) OTOH, a dedicated assignment implementation could avoid an extra allocation that a constructor would have to do always. (Though it's not done here because this is an initial implementation that I'll be expanding further.)

[LB--]

Why should I care about copies but at the same time pretend that a = std::move(a) never happens?

If move assignment is always just swapping the member variables, then I believe self move assignment works fine without needing an explicit check and is effectively harmless - a waste of performance in the very rare case it happens, but that's it.

a dedicated assignment implementation could avoid an extra allocation that a constructor would have to do always

Ah, as in when the capacity is already enough to support the new data? That does make sense as a potential reason to not use copy and swap. Still though, there is a better way to efficiently handle that case while also not needing to check for self assignment, and it would also improve the exception guarantees: if there isn't enough capacity, try to allocate the required capacity and perform the copy first, and then do swaps as the last step. This can be done by re-using the copy ctor and move assignment operator after checking the capacity.