[libcxx] updates the feature-test macro generator

[cjdb]

Summary

D97015 didn't correctly update generate_feature_test_macro_components.py.

[cjdb]

rebases to activate CI (bad gateway)

[• Quuxplusone]

Assuming you've verified that (after this patch) running utils/generate_feature_test_macro_components.py produces no git diffs, where (before this patch) it used to produce diffs, I'd assume this patch is good to go. Maybe one of the other newly minted libc++ approvers can give it the libc++ stamp of approval. :)

[cjdb]

ran script, pushed changes (didn't realise this was a manual process)

[cjdb]

In D97904#2602288, @Quuxplusone wrote:

Assuming you've verified that (after this patch) running utils/generate_feature_test_macro_components.py produces no git diffs, where (before this patch) it used to produce diffs, I'd assume this patch is good to go.

PTAL again.

Maybe one of the other newly minted libc++ approvers can give it the libc++ stamp of approval. :)

Yes please! I didn't see @ldionne's email till after I put this up.

[cjdb]

something went wrong with the previous upload

[cjdb]

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp

4116

This (and elsewhere) feels wrong to me.

[Mordante]

Maybe one of the other newly minted libc++ approvers can give it the libc++ stamp of approval. :)

Yes please! I didn't see @ldionne's email till after I put this up.

I can :-) LGTM.

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp

4116

It looks odd, but it's correct. It has been added in D82171 since the it broke some build bots. The header contains the __floating_point concept. (the std:: added in that patch is no longer there.)

[• Quuxplusone]

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp

4116

I guess you mean the phrasing of the error message itself, right? Yes but I wouldn't worry about it. :)

As for the fact that <numbers> depends on <concepts> in the first place, wow, yeah, that is unfortunate; but I've checked that it is physically accurate: <numbers> does start with the line

#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)

which does literally mean that if the compiler doesn't support concepts then you can't use std::pi.

[• Quuxplusone]

Waaaait a minute. @cjdb I think you wrote !defined in the script when you meant defined. Could you check that, please, and fix, and regenerate, before pushing? But I don't think you need to repost and re-wait for review; let's just get main back in a consistent state already.

[EDIT: Wait x2, now I think I was wrong and !defined is correct, because it's asking if No Concepts is not defined. Anyway, I stand by my last sentence above: let's ship it already, as long as it gets the headers/tests back in sync with the generator.]

[ldionne]

I would also love to see us adding a test that re-generating the feature test macros (and the headers tests too while we're at it) produces no output. That would allow catching these mistakes earlier in the future.

libcxx/utils/generate_feature_test_macro_components.py

425–426

Here, I think you mean:

"depends": "defined(__cpp_concepts) && __cpp_concepts >= 201907L",
"internal_depends": "!defined(_LIBCPP_HAS_NO_CONCEPTS)",

The way I understand it, internal_depends is for macro dependencies for libc++ itself, and depends is for macro dependencies of the test suite.

[• Quuxplusone]

libcxx/utils/generate_feature_test_macro_components.py

425–426

I agree with your understanding; my source-diving suggested to me that these fields should really be named depends->libcxx_test_depends and internal_depends->version_header_depends respectively. I'd be willing to submit that mechanical patch, after this one lands in some form.

No opinion on whether <version> should be testing _LIBCPP_HAS_NO_CONCEPTS or __cpp_lib_concepts directly.

[cjdb]

So are we addressing issues pre-merge or post-merge? A bit confused on this one, sorry.

[• Quuxplusone]

So are we addressing issues pre-merge or post-merge? A bit confused on this one, sorry.

For me personally the priority is to get the generated content back in sync with the generator script one way or another. I am almost at the point of just running the generator script myself and pushing that diff without review, but I feel like that would be antisocial. ;P But be aware that anyone who pushes any other change to libcxx (e.g. implementing some other feature macro) will probably re-run the script and push the diff without noticing that they're doing so. So it's gonna get back in sync somehow, with or without this PR.

It sounds like @ldionne wants to get to the bottom of the "depends/internal_depends" stuff badly enough to mark this "request changes," plus it's @ldionne, so obviously I'll defer to him.
"Request changes", to me, strongly implies "please address this issue pre-merge." (If Louis didn't mean that, he should say so.)

[Mordante]

libcxx/utils/generate_feature_test_macro_components.py

425–426

I had a quick look at it and I think @ldionne's proposal is better. I think it would be good to have a look at this script and improve the naming and also improve the documentation at https://libcxx.llvm.org/docs/DesignDocs/FeatureTestMacros.html.

I also noticed a lit_markup in the script, this seems quite useful to disable all tests using concepts when the compiler doesn't support it.

I'll have a closer look later.

[ldionne]

In D97904#2605361, @cjdb wrote:

So are we addressing issues pre-merge or post-merge? A bit confused on this one, sorry.

My ask was to change to

"depends": "defined(__cpp_concepts) && __cpp_concepts >= 201907L",
"internal_depends": "!defined(_LIBCPP_HAS_NO_CONCEPTS)",

since that's what we really mean. The other feedback about the depends + internal_depends naming is something we can (and should) address separately. Ship it once you've applied the changes explained just above.

Sorry about the lack of clarity in my review yesterday. I tried reviewing a lot of stuff before going OOO for an extended period of time and I think I went a little bit too fast. I should have explained better.

[cjdb]

In D97904#2606538, @ldionne wrote:

In D97904#2605361, @cjdb wrote:

So are we addressing issues pre-merge or post-merge? A bit confused on this one, sorry.

My ask was to change to

"depends": "defined(__cpp_concepts) && __cpp_concepts >= 201907L",
"internal_depends": "!defined(_LIBCPP_HAS_NO_CONCEPTS)",

since that's what we really mean. The other feedback about the depends + internal_depends naming is something we can (and should) address separately. Ship it once you've applied the changes explained just above.

Sorry about the lack of clarity in my review yesterday. I tried reviewing a lot of stuff before going OOO for an extended period of time and I think I went a little bit too fast. I should have explained better.

Gotcha, thanks for spelling it out :-)

[cjdb]

applies @ldionne's feedback

[ldionne]

Has this been checked in? If not, let's do it.

[cjdb]

rebasing to activate CI

libcxx/include/version

 // # define __cpp_lib_is_pointer_interconvertible          201907L
 # if !defined(_LIBCPP_HAS_NO_THREADS)
 // #   define __cpp_lib_jthread                            201911L
 # endif
 # if !defined(_LIBCPP_HAS_NO_THREADS) && !defined(_LIBCPP_AVAILABILITY_DISABLE_FTM___cpp_lib_latch)
 #   define __cpp_lib_latch                              201907L
 # endif
 # define __cpp_lib_list_remove_return_type              201806L
-# ifndef _LIBCPP_HAS_NO_CONCEPTS
+# if !defined(_LIBCPP_HAS_NO_CONCEPTS)
 #   define __cpp_lib_math_constants                     201907L
 # endif
 // # define __cpp_lib_polymorphic_allocator                201902L
 // # define __cpp_lib_ranges                               201811L
 # define __cpp_lib_remove_cvref                         201711L
 # if !defined(_LIBCPP_HAS_NO_THREADS) && !defined(_LIBCPP_AVAILABILITY_DISABLE_FTM___cpp_lib_semaphore)
 #   define __cpp_lib_semaphore                          201907L
 # endif

libcxx/test/std/language.support/support.limits/support.limits.general/numbers.version.pass.cpp

 #elif TEST_STD_VER == 17
 
 # ifdef __cpp_lib_math_constants
 #   error "__cpp_lib_math_constants should not be defined before c++20"
 # endif
 
 #elif TEST_STD_VER == 20
 
-# ifndef _LIBCPP_HAS_NO_CONCEPTS
+# if defined(__cpp_concepts) && __cpp_concepts >= 201907L
 #   ifndef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should be defined in c++20"
 #   endif
 #   if __cpp_lib_math_constants != 201907L
 #     error "__cpp_lib_math_constants should have the value 201907L in c++20"
 #   endif
 # else
 #   ifdef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should not be defined when defined(__cpp_concepts) && __cpp_concepts >= 201907L is not defined!"
 #   endif
-# endif // _LIBCPP_HAS_NO_CONCEPTS
+# endif
 
 #elif TEST_STD_VER > 20
 
-# ifndef _LIBCPP_HAS_NO_CONCEPTS
+# if defined(__cpp_concepts) && __cpp_concepts >= 201907L
 #   ifndef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should be defined in c++2b"
 #   endif
 #   if __cpp_lib_math_constants != 201907L
 #     error "__cpp_lib_math_constants should have the value 201907L in c++2b"
 #   endif
 # else
 #   ifdef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should not be defined when defined(__cpp_concepts) && __cpp_concepts >= 201907L is not defined!"
 #   endif
-# endif // _LIBCPP_HAS_NO_CONCEPTS
+# endif
 
 #endif // TEST_STD_VER > 20
 
 int main(int, char**) { return 0; }

libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp

 
 # ifndef __cpp_lib_map_try_emplace
 #   error "__cpp_lib_map_try_emplace should be defined in c++20"
 # endif
 # if __cpp_lib_map_try_emplace != 201411L
 #   error "__cpp_lib_map_try_emplace should have the value 201411L in c++20"
 # endif
 
-# ifndef _LIBCPP_HAS_NO_CONCEPTS
+# if defined(__cpp_concepts) && __cpp_concepts >= 201907L
 #   ifndef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should be defined in c++20"
 #   endif
 #   if __cpp_lib_math_constants != 201907L
 #     error "__cpp_lib_math_constants should have the value 201907L in c++20"
 #   endif
 # else
 #   ifdef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should not be defined when defined(__cpp_concepts) && __cpp_concepts >= 201907L is not defined!"
 #   endif
-# endif // _LIBCPP_HAS_NO_CONCEPTS
+# endif
 
 # if !defined(_LIBCPP_VERSION)
 #   ifndef __cpp_lib_math_special_functions
 #     error "__cpp_lib_math_special_functions should be defined in c++20"
 #   endif
 #   if __cpp_lib_math_special_functions != 201603L
 #     error "__cpp_lib_math_special_functions should have the value 201603L in c++20"
 #   endif

 
 # ifndef __cpp_lib_map_try_emplace
 #   error "__cpp_lib_map_try_emplace should be defined in c++2b"
 # endif
 # if __cpp_lib_map_try_emplace != 201411L
 #   error "__cpp_lib_map_try_emplace should have the value 201411L in c++2b"
 # endif
 
-# if !_LIBCPP_HAS_NO_CONCEPTS
+# if defined(__cpp_concepts) && __cpp_concepts >= 201907L
 #   ifndef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should be defined in c++2b"
 #   endif
 #   if __cpp_lib_math_constants != 201907L
 #     error "__cpp_lib_math_constants should have the value 201907L in c++2b"
 #   endif
 # else
 #   ifdef __cpp_lib_math_constants
 #     error "__cpp_lib_math_constants should not be defined when defined(__cpp_concepts) && __cpp_concepts >= 201907L is not defined!"
 #   endif
-# endif // !_LIBCPP_HAS_NO_CONCEPTS
+# endif
 
 # if !defined(_LIBCPP_VERSION)
 #   ifndef __cpp_lib_math_special_functions
 #     error "__cpp_lib_math_special_functions should be defined in c++2b"
 #   endif
 #   if __cpp_lib_math_special_functions != 201603L
 #     error "__cpp_lib_math_special_functions should have the value 201603L in c++2b"
 #   endif

libcxx/utils/generate_feature_test_macro_components.py

feature_test_macros = [ add_version_header(x) for x in [
     "name": "__cpp_lib_map_try_emplace",
     "values": { "c++17": 201411 },
     "headers": ["map"],
   }, {
     "name": "__cpp_lib_math_constants",
     "values": { "c++20": 201907 },
     "headers": ["numbers"],
     "depends": "defined(__cpp_concepts) && __cpp_concepts >= 201907L",
-    "internal_depends": "defined(__cpp_concepts) && __cpp_concepts >= 201907L",
+    "internal_depends": "!defined(_LIBCPP_HAS_NO_CONCEPTS)",
   }, {
     "name": "__cpp_lib_math_special_functions",
     "values": { "c++17": 201603 },
     "headers": ["cmath"],
     "unimplemented": True,
   }, {
     "name": "__cpp_lib_memory_resource",
     "values": { "c++17": 201603 },
     "headers": ["memory_resource"],