This improves the naming of the fields depends/internal_depends. It
also adds the documentation for this script. The changes are based on
D99290 and its review comments.
Details
- Reviewers
EricWF ldionne curdeius • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rGc2c68a5940dc: [libc++] Improve generate_feature_test_macro_components.py.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally awesome.
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
33 | @Mordante: I propose that you lose the s in libcxx_guards and test_suite_guards: just do libcxx_guard and test_suite_guard. These are single strings, not arrays of strings; and shorter is better anyway. I would not object if someone dug up a precedent for spelling it libcxx-guard. | |
36–39 | ||
52 | (A) why not? | |
57 | 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? | |
60 | ||
670 | Let's throw in another couple asserts here like assert all(("libcxx_guards" in tc) == ("test_suite_guards" in tc) for tc in feature_test_macros) assert all(key in ["name", "values", "headers", "libcxx_guards", "test_suite_guards", "unimplemented"] for key in tc.keys() for tc in feature_test_macros) The first one allows you to remove the assert from line 755. |
I'm fine with this either as-is or with Arthur's suggestions. I'll let you folks go back-and-forth, but feel free to commit this once Arthur is satisfied.
@Quuxplusone When you accept, please accept as libc++ too.
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
52 |
Because the test suite is supposed to work even on non-libc++ standard libraries. |
Thanks for the reviews, will commit an update shortly.
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
33 | I used the naming Louis proposed in D99290 but I don't mind to make it the name singular. | |
36–39 | I like your wording, but I dislike abbreviations in a text so will replace them. | |
52 |
I'll look at removing some fluff of 44-50, but I think it's good to state a more explicitly what's expected for this field.
Good point. | |
57 | 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)" | |
670 |
This one doesn't work, I adjusted it to a working version. My Python is a bit rusty, so please have a look at it.
I like this.
Me neither, so I'll just keep it. |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
36–39 | 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. | |
57 | 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). |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
36–39 | https://docs.python.org/3/c-api/dict.html has the Dictonary in the title so used that as basis to use dictionary. But I'll change it. | |
57 |
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>.
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. |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
57 | 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/. |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
57 | I'll do that. I think it also explains why it can't depend on <__config>. Explaining why is always good :-) |
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. :)
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
42–58 | I've taken the liberty of providing a suggested edit I'd be happy with. I think anything close to this is probably OK. | |
49 | 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. |
Thanks a lot for your review comments @Quuxplusone, it really helps to get the documentation is a better shape!
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
42–58 | I disagree with the TODO. I think using <__availability> is fine. I'll update the patch with the reason why. |
Addresses review comments. Instead of adding a TODO explain what's valid for
<__availability>
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
52 | This phrase is repeated just before the bullet list. |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
56–59 | Wonky English grammar here; also, this logic applies equally well to any libc++ header (that libc++ headers define things that aren't defined by other library implementations). |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
56–59 | Ah, D94983 enlightens me that the FTM in these macro names stands specifically for "feature-test macro"! # 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 can't depend on macros defined in <__config> because the # `test/std/` parts of the test suite are intended to be # portable to any C++ standard library implementation, not # just libc++. It may depend on # * macros defined by the compiler itself, # * macros generated by CMake, or # In some cases we add `&& !defined(_LIBCPP_AVAILABILITY_DISABLE_FTM_...)` # in order to make libc++ pass the tests on OSX; see D94983. Anyway, I still say ship it and handle the cleanup post-commit. Notice that you've got two copies of libcxx_guard now; delete the second copy. |
libcxx/utils/generate_feature_test_macro_components.py | ||
---|---|---|
49 |
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. |
@Mordante: I propose that you lose the s in libcxx_guards and test_suite_guards: just do libcxx_guard and test_suite_guard. These are single strings, not arrays of strings; and shorter is better anyway.
(Btw, I always interpreted depends as the present-tense singular verb, as in "depends on," not as an abbreviation of "dependencies.")
I would not object if someone dug up a precedent for spelling it libcxx-guard.