This is an archive of the discontinued LLVM Phabricator instance.

[libc++] improve feature test macro script
ClosedPublic

Authored by WimLeflere on Jan 12 2021, 10:07 AM.

Details

Summary

I've been playing a bit with the generate_feature_test_macro_components.py script and replaced some hardcoded values with extra code generation (generate ALL the things).
The output is the same and it makes updating the script less work for the coming 25 C++ standards (until 2 digit number overflow).

Feel free to 'veto' if you think it's overkill.

Diff Detail

Event Timeline

WimLeflere created this revision.Jan 12 2021, 10:07 AM
WimLeflere requested review of this revision.Jan 12 2021, 10:07 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 12 2021, 10:07 AM

Seems like a generally good direction to me, FWIW.

libcxx/utils/generate_feature_test_macro_components.py
904

I see no obvious reason for the special case here. Sure we currently do it as #if TEST_STD_VER < 14, but would anything really break if we did it as #if TEST_STD_VER == 11 instead? It'd be worth trying to find out.

917

I think this should just be assert not std_dialects[-1][-1].isnumeric(), and then simplify accordingly. (That would technologically enforce my earlier opinion that whoever changes 2a to 20 should go ahead and add 2b at the same time.)

924

FWIW, I would recommend copying the style from circa line 800: use template.format("""...""") instead of f-strings, and use one big """ string with all the arguments collected at the bottom, instead of a bunch of little strings append'ed together. (This also solves your question about whether f-strings are allowed, because you'd no longer need to use f-strings.)

1032

This [1:] smells questionable.

Concerning Python version, I don't know of any official policy but given that Python 3.5 is already end-of-life, requiring Python 3.6 seems OK.

ldionne requested changes to this revision.Jan 12 2021, 11:22 AM
ldionne added a subscriber: ldionne.

The direction looks good to me too.

Concerning Python version, I don't know of any official policy but given that Python 3.5 is already end-of-life, requiring Python 3.6 seems OK.

Technically, LLVM decided to support Python 2.7 until January 2021 if I remember correctly. So I think we might be good.

But in all cases, this script is only run by libc++ contributors, and I think it's a more-than-reasonable requirement for that.

libcxx/utils/generate_feature_test_macro_components.py
707

I assume nr stands for number? I would find it a lot easier to read if you used number instead of trying to save a few characters (here and below).

This revision now requires changes to proceed.Jan 12 2021, 11:22 AM
WimLeflere edited the summary of this revision. (Show Details)
  • added assert for last std not numeric
  • replaced f-strings with template.format

replaced nr with number

WimLeflere added inline comments.Jan 12 2021, 12:15 PM
libcxx/utils/generate_feature_test_macro_components.py
904

a special case is also needed for #if vs #elif, so I would keep it

1032

If c++11 is not skipped an empty table section will be generated for c++11 as it has not macros

ldionne added inline comments.Jan 12 2021, 12:41 PM
libcxx/utils/generate_feature_test_macro_components.py
1032

It seems to me like the correct thing to do here is to not include c++11 in the dialects in the first place, then?

removed c++11 from the std dialects and updated the code

Any action required from my side?

ldionne accepted this revision.Jan 18 2021, 11:58 AM

Do you need help committing this? If so, please provide Author Name <email@domain>. Thanks!

This revision is now accepted and ready to land.Jan 18 2021, 11:58 AM

Do you need help committing this? If so, please provide Author Name <email@domain>. Thanks!

Is it an action I can take in the web interface?
Do I need certain access rights?

Info: Wim Leflere wim.leflere@gmail.com

Do you need help committing this? If so, please provide Author Name <email@domain>. Thanks!

Is it an action I can take in the web interface?
Do I need certain access rights?

Info: Wim Leflere wim.leflere@gmail.com

The usual workflow for frequent contributors is to commit themselves as explained in https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. If you plan on submitting a lot of patches, you should consider getting commit access so that we don't have to commit it for you. For now, it's fine of course -- I'll commit this for you.

Thanks!

This revision was automatically updated to reflect the committed changes.