This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor Windows support headers.
ClosedPublic

Authored by EricWF on May 8 2017, 6:34 PM.

Details

Summary

This patch refactors and tries to remove as much of the Windows support headers as possible. This is needed because they currently introduce super weird include cycles and dependencies between STL and libc headers.

The changes in this patch are:

  • remove support/win32/support.h completely. The required parts have either been moved into support/win32/msvc_support.h (for MSVC only helpers not needed by Clang), or directly into their respective foo.h headers.
  • Combine locale_win32.h and locale_mgmt_win32.h into a single headers, this header should only be included within __locale or locale to avoid include cycles.
  • Remove the unneeded parts of limits_win32.h and re-name it to limits_msvc_win32.h since it's only needed by Clang.

I've tested this patch using Clang on Windows, but I suspect it might technically regress our non-existent support for MSVC. Is somebody able to double check?

This refactor is needed to support upcoming fixes to <locale> on Windows.

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.May 8 2017, 6:34 PM
EricWF updated this revision to Diff 98246.May 8 2017, 6:44 PM
  • Rename msvc_support.h to msvc_builtin_support.h.
EricWF planned changes to this revision.May 8 2017, 9:09 PM

This breaks MinGW. I'll update soon.

include/algorithm
647 ↗(On Diff #98246)

This change is incorrect too.

include/limits
114 ↗(On Diff #98246)

This is incorrect for MinGW.

bcraig added inline comments.May 9 2017, 9:12 AM
include/__config
232–235 ↗(On Diff #98246)

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.

compnerd requested changes to this revision.May 9 2017, 4:33 PM
compnerd added inline comments.
include/__config
232–235 ↗(On Diff #98246)

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?

include/stdio.h
113–118 ↗(On Diff #98246)

Should this be hoisted above the #ifdef __cplusplus? This seems like it should be defined by stdio.h but isn't in msvcrt?

include/support/win32/locale_win32.h
24 ↗(On Diff #98246)

Can you please clang-format this block?

include/support/win32/msvc_builtin_support.h
33–51 ↗(On Diff #98246)

I think I prefer the following implementation:

_LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int value) {
  return __popcnt(value);
}
58–76 ↗(On Diff #98246)

I think I prefer the following implementation:

_LIBCPP_ALWAYS_INLINE int __builtin_popcountll(unsigned long long value) {
  return __popcnt64(value);
}
src/string.cpp
430 ↗(On Diff #98246)

This seems scary. Why do we need to cast the function?

majnemer added inline comments.
include/support/win32/msvc_builtin_support.h
33 ↗(On Diff #98246)

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?

EricWF added inline comments.May 9 2017, 5:32 PM
include/__config
232–235 ↗(On Diff #98246)

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.

include/support/win32/msvc_builtin_support.h
33 ↗(On Diff #98246)

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.

33–51 ↗(On Diff #98246)

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.

src/string.cpp
430 ↗(On Diff #98246)

No idea.

EricWF updated this revision to Diff 98528.May 10 2017, 1:48 PM
EricWF edited edge metadata.
  • General cleanup changes.

Libc++ trunk currently doesn't build under MinGW, so I'm not concerned about breaking that worse with this patch. I've set up a Appveyor builder to track MinGW so we can get it green later.

Also I'm planning on making all of the the changes suggested in inline comments, but in a follow up patch. I would like to keep this to simply renaming and restructuring include files.
With that in mind does anybody object to landing this?

EricWF accepted this revision.May 10 2017, 2:09 PM

@compnerd I'm going to go ahead and land this and then submit a separate review for the changes you've requested. I hope you don't mind :-)

This revision was automatically updated to reflect the committed changes.
libcxx/trunk/include/support/win32/msvc_builtin_support.h