Skip to content
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.

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

Replace the fixed-size struct String with reference-counted std::strings #1359

Merged
merged 7 commits into from
Mar 22, 2024

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Mar 18, 2024

We tried this before in #885, now it's finally getting done!

Fixes #650. (No more string length limit!)

Be sure to rebase this, don't squash merge it, so the commits are preserved!

Still to do (but in their own individual PRs):

If we see a performance hit from any of this, there are possible optimizations to try:

  • Use std::move on large parser values whenever possible; avoid copying strings in general.
  • reserve space in strings (or other STL collections) before building them up.
  • Pass std::string_view, not const std::string & (and create these from C string literals or char const * pointers-to-buffers)?
  • Make parser values be std::shared_ptr<std::string>s themselves, instead of self-owned std::strings?
  • Use a custom smart pointer instead of std::shared_ptr, which would be reference-counted but not atomic. (Edit: apparently some implementations of std::shared_ptr, including gcc, are not always atomic!)

@Rangi42 Rangi42 added rgbasm This affects RGBASM refactoring This PR is intended to clean up code more than change functionality optimization This increases performance or decreases size labels Mar 18, 2024
@Rangi42 Rangi42 requested a review from ISSOtm March 18, 2024 18:49
@Rangi42 Rangi42 force-pushed the string branch 4 times, most recently from 679ba3f to 1a14e2b Compare March 19, 2024 05:06
@Rangi42 Rangi42 marked this pull request as draft March 19, 2024 05:06
@ISSOtm ISSOtm force-pushed the string branch 2 times, most recently from a4ce918 to 69356c7 Compare March 19, 2024 08:10
@Rangi42 Rangi42 force-pushed the string branch 2 times, most recently from 1c97c47 to 31bd8a3 Compare March 19, 2024 20:12
@ISSOtm ISSOtm force-pushed the string branch 2 times, most recently from 2f3e0fc to cef6e6f Compare March 21, 2024 00:10
@ISSOtm ISSOtm marked this pull request as ready for review March 21, 2024 00:15
@ISSOtm ISSOtm changed the title [WIP] Replace the fixed-size struct String with reference-counted std::strings Replace the fixed-size struct String with reference-counted std::strings Mar 21, 2024
@Rangi42 Rangi42 force-pushed the string branch 4 times, most recently from 1bcf6e1 to 71cd418 Compare March 21, 2024 16:20
src/asm/fstack.cpp Outdated Show resolved Hide resolved
@Rangi42
Copy link
Contributor Author

Rangi42 commented Mar 22, 2024

This does not yet fix #709; we can't re-enable ASan for RGBASM because there are still memory leaks.

At least some of them stem from these newly allocated strings which are never deleted; probably they should be std::make_shared<std::string>s as well:

src/asm/lexer.cpp:2192:  lexerState->captureBuf = new (std::nothrow) std::vector<char>();
src/asm/parser.y:521:    $$ = new (std::nothrow) MacroArgs();
src/asm/symbol.cpp:504:  std::string_view *macro = new (std::nothrow) std::string_view(body, size);

(Possibly some of the new (std::nothrow) FileStackNodes are being leaked too?)

@Rangi42 Rangi42 merged commit 6a5518e into gbdev:master Mar 22, 2024
24 checks passed
@Rangi42 Rangi42 deleted the string branch March 22, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization This increases performance or decreases size refactoring This PR is intended to clean up code more than change functionality rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow longer strings
2 participants