Skip to content

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 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.
@AZero13 AZero13 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.
@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! 😸 😸 😸 😸

@AZero13 AZero13 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