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

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

Quuxplusone added a subscriber: cjdb.
Quuxplusone added inline comments.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:36-39
+# values            A list of dictionaries. One dictionary contains a C++
+#                   version and the value of the feature-test macro for that
+#                   C++ version. (Note: currently there's no proper support
+#                   when 2 papers in a C++ version modify the value.)
----------------
Mordante wrote:
> Quuxplusone wrote:
> > 
> I like your wording, but I dislike abbreviations in a text so will replace them.
Python calls the type `dict`, though. The situation is analogous to a comment about C++ saying "This parameter is an int" versus "This parameter is an integer" — yes in some sense `int` was once short for "integer," but IMHO it's taken on enough of a life of its own that we should use the more precise technical term.
(If this were JavaScript, I'd equally be pushing to call it an `object` instead of a `dictionary`.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99615