Fix some memory issues with ctl::string #1201
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were a few errors in how capacity and memory was being handled for small strings. The capacity errors meant that small strings would become big strings too soon, and the memory error introduced undefined behavior that was caught by CheckMemoryLeaks in our test file but only sometimes. The crucial change is in reserve: we only copy n bytes into p2, and then we manually set the null terminator instead of expecting it to have been there already. (E.g. it might not be there for an empty small string.) We also fix one other doozy in append when we were exactly at the small- to-big string boundary: we set the last byte (i.e., the remainder field) to 0, then decremented it, giving us size_t max. Whoops. We boneheadedly fix this by setting the 0 byte after we've fixed up the remainder, so it is at worst a no-op. Otherwise, capacity now works the same for small strings as it does with big strings: it's the amount of space available including the null byte. We test all of this with a new test that only gets included if our class under test is not std::string (presumably meaning it's ctl::string.) The test manually verifies that the small string optimization behaves how we expect. Since this test checks against std::string, we go ahead and include that other header from the STL.
This modifies the new test we introduced to also run on std::string, but it just does the append without expecting anything about how its data is stored. We also check that the string has the right value afterwards.
Also runs the test file through clang-format.
jart
approved these changes
Jun 7, 2024
Also it's a bit more idiomatic to say s.npos rather than string::npos.
std::string doesn't have an append(char)
This reverts commit a8dcc1a.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were a few errors in how capacity and memory was being handled for small strings. The capacity errors meant that small strings would become big strings too soon, and the memory error introduced undefined behavior that was caught by CheckMemoryLeaks in our test file but only sometimes.
The crucial change is in reserve: we only copy n bytes into p2, and then we manually set the null terminator instead of expecting it to have been there already. (E.g. it might not be there for an empty small string.)
We also fix one other doozy in append when we were exactly at the small- to-big string boundary: we set the last byte (i.e., the remainder field) to 0, then decremented it, giving us size_t max. Whoops. We boneheadedly fix this by setting the 0 byte after we've fixed up the remainder, so it is at worst a no-op.
Otherwise, capacity now works the same for small strings as it does with big strings: it's the amount of space available including the null byte.
We test all of this with a new test that only gets included if our class under test is not std::string (presumably meaning it's ctl::string.) The test manually verifies that the small string optimization behaves how we expect.
Since this test checks against std::string, we go ahead and include that other header from the STL.