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
----------------
bruno wrote:
> 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...
_LIBCPP_MS_VISUAL_STUDIO_CRT
_LIBCPP_MSVS_CRT
_LIBCPP_MSVC_CRT
_LIBCPP_THE_REAL_MSVC_CRT



https://reviews.llvm.org/D34588



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to