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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
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>
----------------
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)`?


================
Comment at: libcxx/include/exception:104
+// However, <vcruntime_exception.h> does not define std::exception and std::bad_exception 
+// under -fno-exceptions.
+//
----------------
Isn't the `_HAS_EXCEPTIONS` define strictly speaking independent of `-fno-exceptions`? As far as I can see, clang-cl doesn't predefine `_HAS_EXCEPTIONS`, and `vcruntime.h` defines `_HAS_EXCEPTIONS` to 1 unless it has been set by the user/caller.

I.e. I think the comment would be more correct by speaking of `_HAS_EXCEPTIONS` consistently instead of involving `-fno-exceptions`.

Secondly, I see that the structure of comments for the ifdef conditions here, is to have the comment preceding the condition. As there's more than one condition involved, the comment for the second condition `#elif defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0` ends up in the ifdef block for the first condition, so at least to me, it'd be more readable if the comment explaining the condition would be after the condition, i.e. within the ifdef block it explains.


================
Comment at: libcxx/include/exception:129
+
+    _NODISCARD virtual const char* __CLR_OR_THIS_CALL what() const _NOEXCEPT { // return pointer to message string
+        return "unknown exception";
----------------
Should this maybe be using `_LIBCPP_NODISCARD` instead? And `__CLR_OR_THIS_CALL` could maybe be omitted in all cases where libc++ is used... I guess it's a tradeoff between exactly matching the reference to the letter (easing compat if taking this to further untested configurations) vs making the code match libc++ conventions (where attribute macros may be defined slightly differently than in upstream vcruntime).


================
Comment at: libcxx/include/exception:145
+public:
+    __CLR_OR_THIS_CALL bad_exception(const char* _Message = "bad exception") noexcept : exception(_Message) {}
+
----------------
Shouldn't this be using `_NOEXCEPT` instead of plain `noexcept`?


================
Comment at: libcxx/include/exception:147
+
+    __CLR_OR_THIS_CALL ~bad_exception() noexcept override {}
+
----------------
As far as I can see, libc++ typically doesn't use `override`


================
Comment at: libcxx/test/support/test_macros.h:189
+     && !defined(__EXCEPTIONS) \
+     || (defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0)
 #define TEST_HAS_NO_EXCEPTIONS
----------------
Previous versions of this patch had parentheses around the first set of conditions, making it clearer wrt the mixed `&&` and `||`, i.e. `#if (existing_condition && other_existing_condition) || (defined(_HAS_EXCEPTIONS && _HAS_EXCEPTIONS == 0)`


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:568
 
+    - label: "Windows (_HAS_EXCEPTIONS=0)"
+      command: "bash libcxx/utils/ci/run-buildbot clang-cl-noexceptions"
----------------
Since earlier revisions of the patch, we now have both clang-cl and mingw CI configurations, so for clarity, please name this configuration `Clang-cl (_HAS_EXCEPTIONS=0)`, or as @ldionne requested earlier, `Clang-cl (no exceptions)`.


================
Comment at: libcxx/utils/ci/run-buildbot:602
+    generate-cmake-libcxx-win  -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF \
+                               -DLIBCXX_TEST_COMPILER_FLAGS="_HAS_EXCEPTIONS=0" \
+                               -DLIBCXX_TEST_CONFIG="llvm-libc++-shared-clangcl.cfg.in"
----------------
Unfortunately, after switching to the new style libcxx test configs, `LIBCXX_TEST_COMPILER_FLAGS` no longer has an effect. (Until the option is removed, maybe our cmake file could warn if it is set?) So currently, you'd need to duplicate the `llvm-libc++-shared-clangcl.cfg.in` test config file and make a new one which also passes this define.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947