[libcxx-commits] [PATCH] D99615: [libc++] Improve generate_feature_test_macro_components.py.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:49
+#                    * macros generated by CMake, or
+#                    * macros defined in <__availability>.
+#                   It can't depend on <__config> since the test suite should
----------------
Quuxplusone wrote:
> Based on the explanation below, I think this bullet point is wrong: other library vendors won't have libc++'s `<__availability>`. Also, `<__availability>` is included from libc++ top-level headers like `<any>`... but won't be included from other vendors' `<any>`. So I don't see why `<__availability>` is treated any different from `<__config>` here.
> 
> IOW, I don't understand why it's not a bug that we use the libc++-specific macro `_LIBCPP_AVAILABILITY_DISABLE_FTM___cpp_lib_atomic_wait` to control the tests for `__cpp_lib_atomic_wait`.  (I'm content to leave a TODO comment and punt on this, rather than block this PR specifically, because I suspect this is a deep question.)
> 
> Orthogonally, you //should// add a bullet point for `macros defined in "test/support/test_macros.h"`, because we use those a lot in the data table. (E.g. `TEST_STD_VER` and `TEST_HAS_BUILTIN`.) These are kosher in `test_suite_guard` but absolutely not in `libcxx_guard`.
> 
> 
> IOW, I don't understand why it's not a bug that we use the libc++-specific macro _LIBCPP_AVAILABILITY_DISABLE_FTM___cpp_lib_atomic_wait to control the tests for __cpp_lib_atomic_wait. (I'm content to leave a TODO comment and punt on this, rather than block this PR specifically, because I suspect this is a deep question.)

It is indeed a "bug". Normally, I would have added a "mode" for the test suite where it runs but doesn't assume that `atomic_wait` is available. I would then turn that on only when running the tests for back-deploying to a platform that doesn't support it. I didn't do it for simplicity, but it's indeed the correct way to do that IMO.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99615