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]

There is class string test src/common/classes/misc/string_test.cpp. It's very old (like string itself) and definitely does not use boost-related features, but there are checks for most of existing functions in the class. May be instead adding separate test it's better to have single one, sooner of all using boost.

PS. I think keeping performance comparison with std::basic_string does not make sense currently and may be cleaned out. First of all because our string is almost never used in performance-critical paths of execution.

[Noremos]

I have moved the old test to the Boost Test Library. Some tests for the comparison method do not pass.

PathName c = "Aa";
PathName d = "AB";
check(c.compare(d), -1);

StringTest.cpp(530): error: in "StringSuite/StringFunctionalTests/CompareTests1": check c.compare(d) == -1 has failed [7967 != -1]

The result is expected to be 0, -1 or 1, but in reality some positive value is obtained -just like, the result of the memcmp function.

[AlexPeshkoff]

May be use something like:
check(sign(c.compare(d)), -1);

[Noremos]

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. I will fix it

[aafemt]

PS. I think keeping performance comparison with std::basic_string does not make sense currently and may be cleaned out. First of all because our string is almost never used in performance-critical paths of execution.

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

[AlexPeshkoff]

On 3/27/24 13:55, Artyom Abakumov wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/common/tests/MoveStringTest.cpp <#8059 (comment)>: > @@ -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.

[aafemt]

Here you trash memory replacing valid bufferSize with INLINE_BUFFER_SIZE.

[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

[asfernandes]

This test is failing in Android builds (https://github.com/FirebirdSQL/firebird/actions/runs/8513177447/job/23316639529)

It does not look as a good practice to reference this file based in the source path.

I think it's better to create this file as temporary file in runtime inside the test.

[Noremos]

Sorry, missed this check.
Is it allowed to use std::filesystem in tests and the android build?

namespace fs = std::filesystem;
auto tempFilePath = fs::temp_directory_path() / "test.txt";
```

[asfernandes]

I hope it is. It should be present in ndk 22b and we are using 25b.

[Noremos]

Should I create a new PR or do you just cherry pick my postfix?

[asfernandes]

New PR, please.

[Noremos]

#8065