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

Move charconv tables #2125

Merged
merged 33 commits into from
Jul 20, 2022
Merged

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Aug 15, 2021

Resolve #172
Resolve #2347

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner August 15, 2021 09:43
@AlexGuteniev AlexGuteniev changed the title Resolve #172 Move charconv tables Aug 15, 2021
stl/inc/charconv Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Aug 16, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 16, 2021
stl/src/xcharconv_ryu_tables.cpp Show resolved Hide resolved
stl/src/charconv.cpp Outdated Show resolved Hide resolved
stl/inc/charconv Outdated Show resolved Hide resolved
stl/src/charconv.cpp Outdated Show resolved Hide resolved
stl/src/charconv.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Aug 16, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 18, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! I've carefully verified that all of the code movement is preserving the tables and changing only necessary things. I've also verified that mixing TUs calling to_chars() that have been compiled with main and this PR results in successful linking, so I don't believe that we need to rename the individual tables (renaming the specialized struct is good and minimally invasive).

I've also verified that the generators continue to compile. They don't generate exactly what's in the headers now, but the transformations are obvious, and I would definitely prefer to avoid mixing the code movement here with any nontrivial updates to the generators.

I will validate and push changes for the minor issues I noticed.

stl/CMakeLists.txt Outdated Show resolved Hide resolved
stl/CMakeLists.txt Outdated Show resolved Hide resolved
stl/CMakeLists.txt Show resolved Hide resolved
stl/inc/header-units.json Outdated Show resolved Hide resolved
stl/msbuild/stl_base/stl.files.settings.targets Outdated Show resolved Hide resolved
tools/scripts/charconv_generate.cpp Outdated Show resolved Hide resolved
tools/scripts/charconv_tables_generate.cpp Outdated Show resolved Hide resolved
tools/scripts/charconv_tables_generate.cpp Outdated Show resolved Hide resolved
stl/inc/xcharconv_tables.h Outdated Show resolved Hide resolved
stl/src/charconv.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jul 19, 2022
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Jul 19, 2022
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I've had to push an additional commit to work around VSO-1579484 "Standard Library Header Units: charconv refactoring + topo sort = error LNK2019: unresolved external symbol".

stl/src/xcharconv_tables_double.cpp Outdated Show resolved Hide resolved
stl/src/xcharconv_tables_float.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I've pushed changes to remove the /* static */ comments (which caused the float tables to reflow). We have a few occurrences of comments like /* = 0 */ which are questionably useful, but this case is different (the definitions couldn't be for anything other than static data members).

I've also pushed a workaround change, suggested by @cdacamar, which avoids the need to modify the <charconv> usage (although the workaround itself is somewhat more verbose). If we ever need to do this elsewhere before the compiler bug is fixed, the one that doesn't require modifying every usage is better.

@StephanTLavavej StephanTLavavej merged commit 27b006e into microsoft:main Jul 20, 2022
@StephanTLavavej
Copy link
Member

Thanks for this significant throughput improvement - it should substantially reduce the size of the Standard Library Header Units and Modules too! 😻 🚀 🎉

@AlexGuteniev AlexGuteniev deleted the charconv_tables branch July 21, 2022 06:38
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
None yet
5 participants