Implementation of move semantics for Firebird::string

[Noremos]

The Firebird project defines "modern C++" features the ones introduced since C++17. Move semantics is already 10 years old, but it is still avoided in code. It would be nice to start implementing it at least in strings. Pools are difficult to manage in the case of Move semantics, so moving is possible within the same pool.

[AlexPeshkoff]

On 3/27/24 13:55, Artyom Abakumov wrote:

@.**** commented on this pull request.

---

In src/common/tests/MoveStringTest.cpp https://github.com/FirebirdSQL/firebird/pull/8059#discussion_r1540881173:

@@ -0,0 +1,146 @@ +#include "boost/test/unit_test.hpp"

Ok, but originally I wanted to know if the compare method returns the value correctly. From your answer I understand that yes, the problem is in too old test.

Definitely.

I will fix it

Please do.

[aafemt]

Move semantic requires to leave source in valid but undetermined state. There is no point to use exchange in constructor because target is empty and three exchanges in row is not atomic anyway.

[AlexPeshkoff]

On 3/27/24 16:52, Dimitry Sibiryakov wrote:

BTW, if it is not used in performance-critical places, why not to drop them and use |std::string| instead?

It lacks 90% of functions, used inour code. But I suppose you talk about std::basic_string here? Unfortunately it's also missing some useful functions present in our string, like trim() and printf().

Last but not least - if something works why spent time replacing it?

[aafemt]

I'm talking about deriving Firebird::string from std::string adding necessary functions and replacing those that differ from standard by name only. The result would have better portability and new standard features (like this move semantic) will be available out-of-box.

[Noremos]

Move semantic requires to leave source in valid but undetermined state

I'm sorry, but I didn't understand you very well. Are you suggesting to delete these 2 lines?

stringLength = std::exchange(rhs.stringLength, 0);
bufferSize = std::exchange(rhs.bufferSize, INLINE_BUFFER_SIZE);

[aafemt]

I'm suggesting to delete whole baseMove() performing minimal needed operations individually. Move constructor do not have to require initialization of member of this - it is a waste of time.

[AlexPeshkoff]

On 3/28/24 09:35, Artyom Abakumov wrote:

Move semantic requires to leave source in valid but undetermined state

@DS What you've quoted (undetermined == unspecified ?) is how std::containers behave, but that's not an absolute requirement for move semantic.

I'm sorry, but I didn't understand you very well. Are you suggesting to delete these 2 lines?

|stringLength = std::exchange(rhs.stringLength, 0); bufferSize = std::exchange(rhs.bufferSize, INLINE_BUFFER_SIZE); |

Move source to be left in a state, valid for dtor call. Formally we need not modify length/bufsize in the source, i.e.

|stringLength = rhs.stringLength; bufferSize = rhs.bufferSize; | should be enough, but I prefer to be on the safe side and keep the code resetting move source to empty state. May be even add:

rhs.inlineBuffer[0] = '\0';

or hell knows what one gets calling c_str() for such string.

[Noremos]

should be enough, but I prefer to be on the safe side and keep the code resetting move source to empty state.

Using a moved variable is not recommended, but it is still a permitted approach, so I agree with you that it is better to to keep a source string in its ready-to-use state. I believe the optimizer will omit these extra lines if necessary