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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:57
+#                   * when available, a macro defined in `include/__config`,
+#                   * otherwise the same as `test_suite_guards`.
+# unimplemented     An optional Boolean field with the value `True`. This field
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > Is this "otherwise" codepath actually hit at the moment? Could we eliminate this whole codepath by duplicating a little bit of data in the table?
> > > > Not entirely sure what you mean. This means you need to write to write the same value for `ibcxx_guards` and `test_suite_guards`. For example see line 68-69 with both depend on the CMake defined macro `"!defined(_LIBCPP_HAS_NO_THREADS)"`
> > > Oh, I see what you were going for now. I had interpreted this section as saying, "If you provide `libcxx_guard` (with a value based on `include/__config` macros), then the script will use it. Otherwise, the script will use the value of `test_suite_guard` instead." So I was saying, can't we just duplicate the data in the table? which is exactly what old lines 68-69 actually do!
> > > 
> > > So I think you should just change this section to avoid my misinterpretation: I'd say something like
> > > ```
> > > # 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
> > > #                    * macros defined in <__availability>
> > > # libcxx_guard      An optional string field. When this field is provided,
> > > #                   `test_suite_guard` must also be provided. This field is used
> > > #                   only to guard the feature-test macro in <version>. It may
> > > #                   be the same as `test_suite_guard`, or it may depend on
> > > #                   macros defined in <__config>.
> > > ```
> > > 
> > > I guess my next question (about the pre-existing code) is, why would we ever want the two definitions to differ? Maybe @ldionne and @cjdb can recap the situation that led to them being different for `__cpp_lib_math_constants` (and/or `__cpp_lib_char8_t`).
> > > So I think you should just change this section to avoid my misinterpretation: I'd say something like
> > Your version is a bit compacter and seems to convey the same message, so that's nice. I only like to use
> > ```
> > #                    * macros defined by the compiler itself,
> > #                    * macros generated by CMake, or
> > #                    * macros defined in <__availability>.
> > ```
> > 
> > > 
> > > I guess my next question (about the pre-existing code) is, why would we ever want the two definitions to differ? Maybe @ldionne and @cjdb can recap the situation that led to them being different for `__cpp_lib_math_constants` (and/or `__cpp_lib_char8_t`).
> > 
> > For `__cpp_lib_math_constants` we test in the test suite for `defined(__cpp_concepts) && __cpp_concepts >= 201907L"` which should work with all compilers. In `<__config>` we may define `_LIBCPP_HAS_NO_CONCEPTS` so that can be used in `<version>`. Whether or not the macro is defined depends on the value and availability of  `__cpp_concepts`.
> Ah, right, the `libcxx_guard` expression can use things that are defined only inside libc++, whereas `test_suite_guard` needs to be an expression that works on //all// vendors' compilers and libraries, because the generated tests go into `test/std/`, not `test/libcxx/`.
> Can you add a sentence to that effect? Maybe use the word "portable." I'm almost tempted to rename "test_suite_guard" into "portable_guard" — but I think I still agree that its being used //only in the generated tests// is its primary characteristic, and needing to be portable across libraries is sort of a logical corollary of that. (But not an //obvious// corollary, which is why I'm asking you to add a sentence about it!)
I'll do that. I think it also explains why it can't depend on `<__config>`. Explaining why is always good :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99615