Skip to content

chore(core): Refactor SmallString #6138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore(core): Refactor SmallString #6138

wants to merge 2 commits into from

Conversation

@dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Nov 29, 2025 •
edited
Loading

Relevant to #6137

  1. Half of SmallStr code assumed values could be less than kPrefLen, half of it asserted it, parts would've just broke. I kept at DCHECK at assign and simplified all other parts
  2. For some reason, if the length of a new string is greater than the current small str, we just decided to pick a raw allocated (robj) string instead, even if it would fit into a reallocated small string
  3. Simplified CompactObj::GetString() part with encodings

dranikpg
dranikpg commented Nov 30, 2025
View reviewed changes
src/core/compact_object.cc
Comment on lines -1591 to -1595
tl.small_str_bytes += u_.small_str.Assign(encoded);
return;
}

if (taglen_ == SMALL_TAG && encoded.size() <= u_.small_str.size()) {
Copy link
Contributor Author

@dranikpg dranikpg Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the part with encoded.size() <= u_.small_str.size()

@dranikpg dranikpg requested a review from BorysTheDev December 1, 2025 17:58
@dranikpg dranikpg marked this pull request as ready for review December 1, 2025 17:58
BorysTheDev
BorysTheDev approved these changes Dec 2, 2025
View reviewed changes
Copy link
Contributor

@BorysTheDev BorysTheDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand a lot, ask somebody else to review too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BorysTheDev BorysTheDev BorysTheDev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dranikpg @BorysTheDev