[libcxx-commits] [PATCH] D103947: [libcxx] Fix using the vcruntime ABI with _HAS_EXCEPTIONS=0 defined

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

Overall I think this looks quite good now, just a couple minor nits left.



================
Comment at: libcxx/include/exception:101
 
-#if !defined(_LIBCPP_ABI_VCRUNTIME)
+#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS == 1)
+// The std::exception class was already included above, but we're explicit about this condition here for clarity
----------------
I'd prefer `_HAS_EXCEPTIONS != 0` instead of `== 1`.


================
Comment at: libcxx/include/exception:89
+// which we use in order to be ABI-compatible with other STLs on Windows.
+#if defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 1
 #include <vcruntime_exception.h>
----------------
paulkirth wrote:
> mstorsjo wrote:
> > Is this condition strictly necessary? Isn't it the case that if we include the header, under these circumstances, it doesn't provide the definitions we need? I.e. we could still keep including the header even if it doesn't give us anything of value.
> > 
> > (OTOH I guess the condition can be good for clarity?)
> > 
> > Are we sure that `vcruntime.h` which sets the default `_HAS_EXCEPTIONS == 1` has been included at this point? (I guess `cstddef` and `cstdlib` boil down to including that transitively?) Or would we need to make this into `#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)`?
> I was trying to be explicit, but maybe my logic was off here. I believe that `vcruntime.h` should always have been pulled in by this point, but I will try and double check that will always be the case. Regardless,  I think your suggestion of `#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)` is the better approach to take here
> 
> 
> 
> 
Here, you added the `!defined(_HAS_EXCEPTIONS)` in the next condition, but I think we should have it here too, on the very first ifdef.

Additionally, I think I'd rather check `_HAS_EXCEPTIONS != 0` instead of `_HAS_EXCEPTIONS == 1` as I believe the define is interpreted as a boolean, if someone were to set it to something else than 0 or 1.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103947/new/

https://reviews.llvm.org/D103947