[PATCH] D32988: [libc++] Refactor Windows support headers.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added inline comments.


================
Comment at: include/__config:232-235
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
----------------
bcraig wrote:
> I can see this helping when we are build libc++, but I don't think it helps client apps.  They could have included windows.h before including our headers.
> 
> Don't get me wrong, I dislike the min and max macros, and bear no hard feelings towards people that define this project wide on the command line, I'm just not sure it will get things done right here.
> 
> In the past, I've just surrounded my min and max declarations with parenthesis to suppress macro expansion.
Yeah, I don't think that this should be defined in `__config`.  Can we do something like `#pragma push_macro` and `#pragma pop_macro` in the necessary files?


================
Comment at: include/stdio.h:113-118
 #if defined(_LIBCPP_MSVCRT)
-extern "C++" {
-#include "support/win32/support.h"
+extern "C" {
+int vasprintf(char **sptr, const char *__restrict fmt, va_list ap);
+int asprintf(char **sptr, const char *__restrict fmt, ...);
 }
 #endif
----------------
Should this be hoisted above the `#ifdef __cplusplus`?  This seems like it should be defined by `stdio.h` but isn't in msvcrt?


================
Comment at: include/support/win32/locale_win32.h:24
+#define LC_MESSAGES_MASK _M_MESSAGES
+#define LC_ALL_MASK (  LC_COLLATE_MASK \
+                     | LC_CTYPE_MASK \
----------------
Can you please clang-format this block?


================
Comment at: include/support/win32/msvc_builtin_support.h:33-51
+_LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int x)
+{
+  // Binary: 0101...
+  static const unsigned int m1 = 0x55555555;
+  // Binary: 00110011..
+  static const unsigned int m2 = 0x33333333;
+  // Binary:  4 zeros,  4 ones ...
----------------
I think I prefer the following implementation:

    _LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int value) {
      return __popcnt(value);
    }


================
Comment at: include/support/win32/msvc_builtin_support.h:58-76
+_LIBCPP_ALWAYS_INLINE int __builtin_popcountll(unsigned long long x)
+{
+  // Binary: 0101...
+  static const unsigned long long m1 = 0x5555555555555555;
+  // Binary: 00110011..
+  static const unsigned long long m2 = 0x3333333333333333;
+  // Binary:  4 zeros,  4 ones ...
----------------
I think I prefer the following implementation:

    _LIBCPP_ALWAYS_INLINE int __builtin_popcountll(unsigned long long value) {
      return __popcnt64(value);
    }


================
Comment at: src/string.cpp:430
 #else
-    return static_cast<int (__cdecl*)(wchar_t* __restrict, size_t, const wchar_t*__restrict, ...)>(swprintf);
+    return static_cast<int (__cdecl*)(wchar_t* __restrict, size_t, const wchar_t*__restrict, ...)>(_snwprintf);
 #endif
----------------
This seems scary.  Why do we need to cast the function?


https://reviews.llvm.org/D32988