Replace the use of _WIN32 in libc++. Replace most use with a C runtime check _LIBCPP_MSVCRT or the new _LIBCPP_WIN32 to indicate that we are using the Win32 API. Use a new _LIBCPP_WCHAR_IS_UCS2 to indicate that we are on an environment that has a short wchar_t.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Sweet.
include/__config | ||
---|---|---|
158 | Perhaps _LIBCPP_WIN32API instead, to be clear that this is specific to the usage of Win32 APIs, rather than just a general catch-all libc++ on Windows macro? | |
798 | Windows has catopen? | |
include/type_traits | ||
1684 | Might be cleaner to have a _LIBCPP_COFF macro (both here and for the similar __config change), to make the intent clearer? |
I don't think wchar_t is UCS-2 anymore, I read somewhere that they switched to UTF-16 as of Windows 2000.
Is it the actual encoding you're interested in, or the element size? Judging from the diff, it looks like both, but the code appears to assume UCS-2, so it could already be broken.
Maybe you could phrase it as _LIBCPP_16BIT_WCHAR or _LIBCPP_SHORT_WCHAR instead?
include/__config | ||
---|---|---|
158 | +1 for that name, since it is self documenting. | |
798 | This is nested in a block that already excludes windows. | |
include/type_traits | ||
1684 | +1 for _LIBCPP_COFF or similar. I would rather explicitly exclude coff than include a list of supported formats. | |
src/thread.cpp | ||
27 | I wonder if the drivers on solaris define __unix__. |
I figured that using the explicit encoding is nicer since it means that its more documenting. I dont have a problem with _LIBCPP_SHORT_WCHAR as that maps quite well to -fshort-wchar.
Update for _LIBCPP_WIN32API and addition of _LIBCPP_OBJECT_FORMAT_COFF, _LIBCPP_OBJECT_FORMAT_ELF, _LIBCPP_OBJECT_FORMAT_MACHO
I think @kimgr's point was that if you're using an explicit encoding, it should be UTF-16 rather than UCS-2 :)
@smeenai I think I tried to make a jumbled mess of points :-)
- Associating an encoding with a char/element type seems wrong; any number of encodings can use wchar_t.
- Windows allegedly uses UTF-16 now, not UCS-2
- There might be a bug in locale.cpp if it actually does UCS-2 -> UTF-8 conversion and Windows no longer uses UCS-2 (though I think they overlap for a lot of code-points)
@compnerd _LIBCPP_SHORT_WCHAR sounds good to me, especially in light of the front-end flag (didn't know that existed.)
Another belated thought: it all depends on the intended meaning of the symbol:
- Does it say anything about the element size (i.e. sizeof(wchar_t)?
- Does it specify the native encoding of wide-char strings for a platform (i.e. UTF-16 on Windows)?
It looks like the same symbol is now used for both things, which may not be a good idea.
I think that it will suffice for now. We can change the flag to be more granular in the future. I think that this is going well beyond the original intent of cleaning up the usage of the _WIN32 macros at this point.
I agree. Since the current set of encodings all have different element sizes and this is not likely to change, this looks good.
include/support/win32/support.h | ||
---|---|---|
113 | Windows wouldn't have __LP64__ defined (since it's LLP64 anyway). According to MSDN _BitScanForward64 is available on x64 and ARM, so the conditional should reflect that (i.e. the existing conditional was incorrect, but since you're changing it you may as well fix it). | |
153 | Same __LP64__ comment here. |
I like this. A lot.
I'm a bit concerned about @smeenai 's comments about __LP64_, and @EricWF 's comment about solaris.
This patch accomplishes (or maybe just moves closer, I need to check) to a goal of mine, which is to have no references to _WIN32 in any header files other that <__config> (and maybe in support/)
LGTM. My comment is a suggestion, not a requirement.
include/support/win32/support.h | ||
---|---|---|
112 | I would be tempted to add another macro here. Something like: #if (defined(_M_ARM) || defined(__arm__)) || \ (defined(_M_AMD64) || defined(__x86_64__)) #define _LIBCPP_WIN_HAS_BITSCAN #endif to avoid repeating the "four-armed combo" |
SVN r290910. I like your idea for an additional BITSCAN macro. I think that Id rather do that in a follow up.
Perhaps _LIBCPP_WIN32API instead, to be clear that this is specific to the usage of Win32 APIs, rather than just a general catch-all libc++ on Windows macro?