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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/__config:232-235
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
----------------
compnerd wrote:
> 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?
Alright I'll remove the `NOMINMAX` define in `__config` and start to sprinkle the `__undef_min_max` include where it's currently needed.

@compnerd, using `push_macro` and `pop_macro` are going to be the correct way to handle this, but that's a change for another patch.


================
Comment at: include/support/win32/msvc_builtin_support.h:33
+
+_LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int x)
+{
----------------
majnemer wrote:
> compnerd wrote:
> > I think I prefer the following implementation:
> > 
> >     _LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int value) {
> >       return __popcnt(value);
> >     }
> I think it'd be better not to call it `__builtin_anything`. MSVC uses the __builtin_ namespace too, see https://godbolt.org/g/HwMskX
> 
> Maybe create a wrapper called `__libcpp_popcount`?
That's a change for another patch, one that's just meant to restructure the headers.

This patch simply moves this code to another file.


================
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 ...
----------------
EricWF wrote:
> majnemer wrote:
> > compnerd wrote:
> > > I think I prefer the following implementation:
> > > 
> > >     _LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int value) {
> > >       return __popcnt(value);
> > >     }
> > I think it'd be better not to call it `__builtin_anything`. MSVC uses the __builtin_ namespace too, see https://godbolt.org/g/HwMskX
> > 
> > Maybe create a wrapper called `__libcpp_popcount`?
> That's a change for another patch, one that's just meant to restructure the headers.
> 
> This patch simply moves this code to another file.
That's a change for another patch, one that's just meant to restructure the headers.

This patch simply moves this code to another file.


================
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
----------------
compnerd wrote:
> This seems scary.  Why do we need to cast the function?
No idea.


https://reviews.llvm.org/D32988