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

Compare the _W_decimal_point of the localeconv, not the decimal_point of said localeconv #3085

Merged
merged 9 commits into from
Sep 13, 2022

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Sep 7, 2022

Since s is a pointer to a wchar_t, it makes more sense to use the wchar_t field of the localeconv struct returned. decimal_point and _W_decimal_point are identical for all supported locales, so this is not a behavioral change.

Additional cleanups, keeping narrow xstoxflt.cpp and wide xwstoxfl.cpp in sync:

  • Move the definition of int word; down; it doesn't need to be initialized because it's always assigned.
  • Upgrade digits and vals to constexpr. Consistently comment them at the end. Consistently use a string literal to initialize digits, which provides a null terminator.
  • Narrow xstoxflt.cpp should use a while-loop, as wide xwstoxfl.cpp was already doing.
    • This is doing something complicated and less structured, so the while-loop is more appropriate.
  • if (seen) { ... } followed by if (!seen) { ... } can simply use else, as we don't modify seen here.

… of the localeconv

Since s is a pointer to a w_chart, it makes more sense to use the wide_char field of the localeconv struct returned.
@AreaZR AreaZR requested a review from a team as a code owner September 7, 2022 13:13
This is doing something complicated and less structured,
so the `while`-loop is more appropriate.
xstoxflt.cpp: Add comment to `digits`. Move `vals` comment to end.

xwstoxfl.cpp: Use a wide string literal to initialize `digits`, which provides the null terminator. Move comments to end.
stl/src/xwstoxfl.cpp Show resolved Hide resolved
stl/src/xwstoxfl.cpp Outdated Show resolved Hide resolved
stl/src/xwstoxfl.cpp Outdated Show resolved Hide resolved
stl/src/xwstoxfl.cpp Outdated Show resolved Hide resolved
stl/src/xwstoxfl.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks - I've pushed the following changes to resolve code review feedback, also getting xstoxflt.cpp and xwstoxfl.cpp synced up.

  • Move the definition of int word; down.
  • Also upgrade vals to constexpr.
  • Style: Attach } else {.
  • xstoxflt.cpp: Mirror } else { change.
  • xstoxflt.cpp: Mirror int word; change.
  • xstoxflt.cpp: Mirror constexpr change.
  • xstoxflt.cpp: (pre-existing) Mirror while-loop.
    • This is doing something complicated and less structured, so the while-loop is more appropriate.
  • (pre-existing) Harmonize narrow/wide definitions of digits/vals.
    • xstoxflt.cpp: Add comment to digits. Move vals comment to end.
    • xwstoxfl.cpp: Use a wide string literal to initialize digits, which provides the null terminator. Move comments to end.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Sep 8, 2022
@StephanTLavavej StephanTLavavej self-assigned this Sep 12, 2022
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 16f50a9 into microsoft:main Sep 13, 2022
@StephanTLavavej
Copy link
Member

Thanks for this code cleanup! 😸 😸 😸 😸

@AreaZR AreaZR deleted the xwstoxfl branch September 26, 2022 16:07
CaseyCarter pushed a commit to CaseyCarter/STL that referenced this pull request Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants