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

Fix boolalpha extraction to be case-sensitive again #1543

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

StephanTLavavej
Copy link
Member

Fixes #1541.

_Getloctxt() is a non-member function template. We can change its signature because it's header-only. (I verified with dumpbin /exports that it isn't exported from the DLL. It's called by separately compiled virtual member functions, which is totally fine. As an aside, this means that the __CRTDECL is unnecessary; I will consider a global cleanup later.)

For clarity, I'm introducing enum class _Case_sensitive (as bool arguments are generally unreadable) and making it mandatory (no default argument).

As a targeted cleanup, because I'm already changing the signature (and this is always called with template argument deduction), I'm reordering the template parameters to match the function parameters.

The core fix is a multi-line conditional expression, but due to the naming I believe it's readable. This seemed less invasive than restructuring the control flow. (I also felt that testing this at runtime was okay, compared to templating on _Case_sensitive and using if constexpr.)

Then we need to change all callsites (not searching for these is what led to the regression; a useful lesson). boolalpha must be case-sensitive, but all of the <xloctime> calls are correctly case-insensitive. (Future cleanup: ":AM:am:PM:pm" is an unnecessary relic now that _Getloctxt() handles lowercase, uppercase, and mixed case.)

Finally, I'm adding the test case that I developed, which is expressed with my new favorite technique, static tables.

@StephanTLavavej StephanTLavavej added bug Something isn't working high priority Important! labels Dec 18, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 18, 2020 01:07
@mnatsuhara mnatsuhara self-assigned this Jan 6, 2021
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Sorry I forgot this one was on my plate! 🍽️ Looks great!

@StephanTLavavej StephanTLavavej merged commit b6ee7a6 into microsoft:master Jan 15, 2021
@StephanTLavavej StephanTLavavej deleted the boolalpha branch January 15, 2021 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Important!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<xlocale>: boolalpha extraction should be case-sensitive
3 participants