[PATCH] D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT
Ben Craig via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 26 18:57:00 PDT 2017
bcraig added a comment.
LGTM, but you should probably get approval from somebody a bit more senior with the project.
Comment at: include/__config:234-235
+// a MS compatibility version is specified.
# ifndef __MINGW32__
-# define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
+# ifdef _MSC_VER
+# define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
> bcraig wrote:
> > majnemer wrote:
> > > compnerd wrote:
> > > > smeenai wrote:
> > > > > You can combine this into just
> > > > >
> > > > > ```
> > > > > # if defined(_MSC_VER) && !defined(__MINGW32__)
> > > > > ```
> > > > >
> > > > > I don't know if `__MINGW32__` and `_MSC_VER` will ever be compiled simultaneously. (clang never defines `_MSC_VER` for its MinGW triples, for example.)
> > > > What if MinGW is built with clang/c2 and MSVC extensions? I think that the two could be defined together. What about cygwin and clang/c2? I guess we can ignore that since cygwin is not under active development.
> > > >
> > > > I think this really goes back to my idea for an additional flag to indicate the C library in use. We can interpret it from the canonicalized triple that LLVM/clang use.
> > > clang/c2 is dead.
> > At some point, I would like to see (or will need to introduce) a flag for which Windows C library is in use (so I'm agreeing with / echoing @compnerd). What all options are there right now? There's the Visual Studio C-runtime (multiple versions), there's msvcrt (used by the OS and mingw), there's the ancient crtdll that we shouldn't ever support, and there's the kernel C runtime (I'm probably the only person that cares about that).
> > I will note that I don't like the name of the macro here. _LIBCPP_MSVCRT implies that msvcrt.dll is being used, when it isn't. I don't think that this patch needs to fix that naming though.
> Any suggestion on a new name instead of `_LIBCPP_MSVCRT` for a future patch?
_LIBCPP_MS_UCRT? (UCRT for universal C-runtime). That's more accurate than MSVCRT, thought it still isn't entirely accurate, as the UCRT is only a subset of the full CRT.
Other brainstormed names...
More information about the cfe-commits