Skip to content

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
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Aug 16, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 16, 2021
@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.

@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".

@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

Development

Successfully merging this pull request may close these issues.

<charconv>, <functional>: #if 0-ed code generation scripts in public headers <charconv>: Lookup tables should be separately compiled

5 participants