Page MenuHomePhabricator

[libcxx] updates the feature-test macro generator
ClosedPublic

Authored by cjdb on Mar 3 2021, 6:14 PM.

Details

Summary

D97015 didn't correctly update generate_feature_test_macro_components.py.

Diff Detail

Event Timeline

cjdb created this revision.Mar 3 2021, 6:14 PM
cjdb requested review of this revision.Mar 3 2021, 6:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 6:14 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 328015.Mar 3 2021, 9:27 PM

rebases to activate CI (bad gateway)

Quuxplusone accepted this revision as: Quuxplusone.Mar 3 2021, 9:34 PM

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 updated this revision to Diff 328018.Mar 3 2021, 9:42 PM

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

cjdb added a comment.Mar 3 2021, 9:42 PM

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 updated this revision to Diff 328019.Mar 3 2021, 9:45 PM

something went wrong with the previous upload

cjdb added inline comments.Mar 3 2021, 9:46 PM
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp
4116

This (and elsewhere) feels wrong to me.

Mordante accepted this revision.Mar 4 2021, 11:22 AM

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

This revision is now accepted and ready to land.Mar 4 2021, 11:22 AM
Quuxplusone added inline comments.Mar 4 2021, 1:02 PM
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 accepted this revision.Mar 4 2021, 1:03 PM
Quuxplusone added a comment.EditedMar 4 2021, 1:07 PM

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 requested changes to this revision.Mar 4 2021, 3:12 PM

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.

This revision now requires changes to proceed.Mar 4 2021, 3:12 PM
Quuxplusone added inline comments.Mar 4 2021, 3:17 PM
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 added a comment.Mar 4 2021, 7:44 PM

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

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 added inline comments.Mar 4 2021, 11:43 PM
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 accepted this revision.Mar 5 2021, 6:14 AM

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.

This revision is now accepted and ready to land.Mar 5 2021, 6:14 AM
cjdb added a comment.Mar 5 2021, 7:13 PM

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 updated this revision to Diff 328721.Mar 5 2021, 7:26 PM

applies @ldionne's feedback

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

cjdb updated this revision to Diff 331326.Mar 17 2021, 11:14 AM

rebasing to activate CI

This revision was automatically updated to reflect the committed changes.