Skip to content

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