This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Update contributor documentation.
ClosedPublic

Authored by Mordante on Mar 24 2021, 12:03 PM.

Details

Reviewers
EricWF
ldionne
Group Reviewers
Restricted Project
Commits
rG48fa06f70b07: [libc++] Update contributor documentation.
Summary

The document has the following updates:

  • Rename 'feature test' to 'feature-test', the latter is the spelling used in the Standard.
  • Improve documentation regarding how to add new feature-test macros in generate_feature_test_macro_components.py. Recently there was some confusion regarding the fields depends and internal_depends.
  • Add information how an ABI list can be downloaded from Buildkite.

@EricWF I would be grateful if you can have a look at the changes
regarding the generate_feature_test_macro_components.py documentation
since you wrote the original version.

Diff Detail

Event Timeline

Mordante requested review of this revision.Mar 24 2021, 12:03 PM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 12:03 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks a lot! Some comments but basically LGTM.

libcxx/docs/Contributing.rst
66

I think we should also change those names. I'd suggest:

depends => test_suite_guards
internal_depends => libcxx_guards

or something along those lines. WDYT?

Typos and co.

libcxx/docs/Contributing.rst
51

Isn't it a dictionary (or however it's called in pythonesque)?

64

Updated on/for?

65
68
69
Quuxplusone added inline comments.
libcxx/docs/Contributing.rst
39–45

I'd add a paragraph here, something like: "Running utils/generate_feature_test_macro_components.py should never generate diffs in a clean checkout; feel free to run it in your local checkout any time you want."

54

Note that values and headers are both coarser-grained than we really need, these days. Because the macros really correspond to papers, not versions of the standard; and the values can have multiple appropriate settings depending on which paper(s) are implemented. This is a long-standing problem with the script, and totally not your doing. But if we're updating the README to explain about the script, I think it should at least gesture in the direction of "these variables do not directly correspond to anything in WG21's system of feature macros and some fudging may be necessary."

65–66
66

We've bikeshedded the names before, but I like @ldionne's suggested names even better than anything we came up with last time around. I say do it!

69
73

This should perhaps say "...when a feature is fully unimplemented. Once implemented enough to turn on the feature-test macro, the field should be removed."
"Fully unimplemented" is different from "not fully implemented" in the case where the same macro has multiple possible values, and we've implemented the lower value but not the higher value.
However, I guess you're thinking more about the case where one iteration of one single "feature" is still so gigantic that it takes multiple commits to get in; then the last commit (not the first) should be to turn on the feature-test macro.

What would folks think about moving the documentation of the various fields into the script itself? That would make it closer to the place where it's used and would be both more discoverable (i.e. you see the documentation whenever you try to modify the script).

What would folks think about moving the documentation of the various fields into the script itself?

I like that idea (in a separate PR, thus making this one smaller).
Adding internal comments to the script could be coupled with renaming the depends/internal_depends fields to test_suite_guards/libcxx_guards so that hopefully they need less-verbose documentation.

What would folks think about moving the documentation of the various fields into the script itself?

I like that idea (in a separate PR, thus making this one smaller).
Adding internal comments to the script could be coupled with renaming the depends/internal_depends fields to test_suite_guards/libcxx_guards so that hopefully they need less-verbose documentation.

I'm in favour of these suggestions. Thanks!

libcxx/docs/Contributing.rst
39–45

Valid point. I already want to look at a way to let the CI test this and fail if the output changes. Like we do with the abilists. Of course it would still be good to add to the documentation.

51

Yes, will change it.

54

I'm aware of this issue, just tried to ignore it in the documentation. I don't think we have a good solution for it at the moment. And I'm not sure what the best solution will be, but I think it would be best to tackle it when it becomes an issue. Or do we already have this issue?

66

I like to suggestion to rename these fields. However I feel that should not be done as part of a documentation update. So I'll create a separate patch.

73

Yes I'm thinking about large features like ranges, concepts, format. I'm not sure what the policy is when we have a feature that's fully implemented for C++17, but not implemented at all for C++20. I think the script can't handle that.
In that case it might be required to adjust the script to have a implemented flag per C++ version, so make it part of values.

libcxx/docs/Contributing.rst
51

also: /marco/macro/ on line 47

54

This past month (i.e. since I started paying attention) I've seen D96385:

"name": "__cpp_lib_constexpr_string",
"values": { "c++20": 201811 },  # because P1032R1 is implemented; but should become 201907 after P0980R1
"headers": ["string"],

and also D97394:

"name": "__cpp_lib_variant",
"values": { "c++17": 202102 },
"headers": ["variant"],
Mordante added inline comments.Mar 30 2021, 10:45 AM
libcxx/docs/Contributing.rst
54

Thanks for the examples Arthur!
In that case maybe we should look at how to add support for this. I assume it can happen that:
P1234 adds __cpp_lib_foo 202103L and isn't implemented.
P1235 adds __cpp_lib_foo 202106L and is implemented.
Which will be interesting.

Since the issue seems more realistic than I expected I think we should think of a solution. IMO this means we need to modify the script. However I think that should be a separate commit and we should have a discussion about the requirements before creating a solution.

Mordante marked 7 inline comments as done.Mar 30 2021, 11:28 AM

Mark some parts as done. These will be part of a separate review where the documentation for utils/generate_feature_test_macro_components.py will become part of the script and contain @ldionne's suggested renaming of the fields.

libcxx/docs/Contributing.rst
73

I think the current wording is correct when the feature-test macro has one possible value. Otherwise we can't properly support it yet. As replied to Arthur's comment I think we should support this, but that should be a separate patch.

ldionne accepted this revision.Mar 30 2021, 2:56 PM

LGTM if you remove the (now duplicate) table that was moved to the script in https://reviews.llvm.org/D99615.

This revision is now accepted and ready to land.Mar 30 2021, 2:56 PM
Mordante marked 7 inline comments as done.Apr 4 2021, 2:59 AM

Thanks for the comments. I'll update the patch for a final review.

Mordante updated this revision to Diff 335140.Apr 4 2021, 3:02 AM
  • Remove the documentation future-test script documentation. It's moved to the script in D99615.
  • Replaced one with you.
  • Addresses the review comments.
ldionne accepted this revision.Apr 6 2021, 11:55 AM
This revision was automatically updated to reflect the committed changes.