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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM at this point, modulo my remaining comments (and my suggested edit resolves all of my remaining comments).
I'd say it makes sense to get this landed expeditiously and deal with the TODOs separately.

Please make sure the script still runs and produces no unexpected git diffs, of course. :)



================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:42-58
+# test_suite_guard  An optional string field. When this field is provided,
+#                   `libcxx_guard` must also be provided. This field is used
+#                   only to generate the unit tests for the feature-test macros.
+#                   It must never depend on anything defined in <__config>,
+#                   but it may depend on
+#                    * macros defined by the compiler itself,
+#                    * macros generated by CMake, or
----------------
I've taken the liberty of providing a suggested edit I'd be happy with. I think anything close to this is probably OK.


================
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
----------------
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`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99615