Skip to content

Conversation

adr26
Copy link
Contributor

@adr26 adr26 commented Aug 25, 2021

Fix references to locale_t to use _locale_t instead.

@PRMerger20
Copy link
Contributor

@adr26 : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@PRMerger20 PRMerger20 requested a review from TylerMSFT August 25, 2021 09:20
@adr26 adr26 changed the title Locale Fix locale types Aug 25, 2021
@ktoliver ktoliver added the aq-pr-triaged Tracking label for the PR review team label Aug 25, 2021
@TylerMSFT
Copy link
Collaborator

@adr26, can you tell me a little bit about the motivation for this change?

@adr26
Copy link
Contributor Author

adr26 commented Aug 25, 2021

@adr26, can you tell me a little bit about the motivation for this change?

Sure @TylerMSFT: I was constructing some test code for CRT printf APIs, after I spotted some errors in the documentation of their behaviour (I'll open another PR for this).

I did a search throughout the CRT code for locale_t, the only hit was in strsafe.h — and then the hit was only in comments on the function signatures, rather than the types of the arguments themselves!

Everywhere else, _locale_t is the type used in the APIs (it being a typedef of __crt_locale_pointers in corecrt.h), and there is no typedef for locale_t.

As such, all the references to locale_t in these docs are incorrect and need to be fixed to use _locale_t instead. Searching for a locale_t typedef in the CRT sources and verifying it doesn't exist may be the easiest way to confirm this assertion is correct, and similarly you can consider that:

#include <locale.h>

int main()
{
    locale_t loc = _get_current_locale();
    return 0;
}

fails to compile:

C:\Temp>cl locale.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.30.30423 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

locale.c
locale.c(5): error C2065: 'locale_t': undeclared identifier
locale.c(5): error C2146: syntax error: missing ';' before identifier 'loc'
locale.c(5): error C2065: 'loc': undeclared identifier
locale.c(5): warning C4047: '=': 'int' differs in levels of indirection from '_locale_t'

whereas:

#include <locale.h>

int main()
{
    _locale_t loc = _get_current_locale();
    return 0;
}

compiles just fine!

C:\Temp>cl locale.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.30.30423 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

locale.c
Microsoft (R) Incremental Linker Version 14.30.30423.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:locale.exe
locale.obj

However, if you need more evidence, you may also individually check each change I've made. For example, the first change is to the signature of _atodbl_l in docs/c-runtime-library/reference/atodbl-atodbl-l-atoldbl-atoldbl-l-atoflt-atoflt-l.md, updating the text to read:

int _atodbl_l ( _CRT_DOUBLE * value, char * str, _locale_t locale );

instead of:

int _atodbl_l ( _CRT_DOUBLE * value, char * str, locale_t locale );

If you compare this with the actual declaration from the CRT sources (in this instance, in stdlib.h), you see that the sources use _locale_t, and not locale_t:

_Check_return_
_ACRTIMP int __cdecl _atodbl_l(
    _Out_    _CRT_DOUBLE* _Result,
    _In_z_   char*        _String,
    _In_opt_ _locale_t    _Locale
    );

You may likewise repeat this exercise manually for the other 109 instances, over 50 files, where I have fixed this: should you so desire... (though hopefully I've convinced you that this is correct without you having to check each one!)

Thanks, Andrew R

@colin-home
Copy link
Contributor

@TylerMSFT I forgot to hit the comment button after writing a note earlier: I checked it out and it's legit. I couldn't tell when that type's name was changed, or if the leading underscores got stripped at some point in our many doc system migrations. I suspect the name change in the Microsoft library dates back to when the POSIX renaming happened twenty years ago. Other libraries support it without the leading underscore, either as an alias or a typedef. We haven't used locale_t in headers since at least 2008, the oldest set of headers on my machine.

@TylerMSFT
Copy link
Collaborator

@adr26 , I just finished checking this out as well and your change is approved :-) Thank you very much for doing that pass! That's incredibly helpful. Here's a little bonus material based on a conversation I just had with a dev here about this:

"pretty much everything that is POSIX or MSVC-specific we prepend an underscore in front of the name. The reason is because to be standards-compliant with C, we can’t include any non-standard symbols in the standard headers, unless they use a reserved name (any name starting with an underscore). For example, if we didn’t do this and you declared a global variable named “open” or “y0”, you would fail to compile, but standard-conforming C should be allowed to do that. Nowadays, we do still declare these unless STDC is 1 (which it never is because it’s tied to /Za) or _CRT_DECLARE_NONSTDC_NAMES is defined.

I believe we put an underscore in front of locale_t for the same reason – it’s not a part of the C standard. For the function names, we also provide oldnames.lib to allow omitting the underscore, but I don’t believe we have anything like this for _locale_t. I imagine the reason is like you said – wasn’t accepted into POSIX until 2008 and at that point it wasn't a priority to use ‘locale_t’ on Windows. Additionally, any C++ users might have a bad time with function overloading if we change type names based on macros like that."

Copy link
Collaborator

@TylerMSFT TylerMSFT left a comment

Choose a reason for hiding this comment

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

You are awesome for doing this sweeping update. Thank you!

@ktoliver ktoliver merged commit e5d743a into MicrosoftDocs:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants