[libc++] Update contributor documentation.

[Mordante]

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.

[ldionne]

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?

[curdeius]

Typos and co.

libcxx/docs/Contributing.rst

51

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

63

Updated on/for?

64

67

68

[• Quuxplusone]

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

64–65

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!

68

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.

[ldionne]

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

[• Quuxplusone]

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.

[Mordante]

In D99290#2650799, @Quuxplusone wrote:

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.

[• Quuxplusone]

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]

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]

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]

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

[Mordante]

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

[Mordante]

• Remove the documentation future-test script documentation. It's moved to the script in D99615.
• Replaced one with you.
• Addresses the review comments.

libcxx/docs/Contributing.rst

 =================
 
 After branching for an LLVM release:
 
 1. Update ``_LIBCPP_VERSION`` in ``include/__config``
 2. Update the ``include/__libcpp_version`` file
 3. Update the version number in ``docs/conf.py``
 
-Modifying feature test macros
+Modifying feature-test macros
 =============================
 
-When adding or updating feature test macros, you should update the corresponding tests.
-To do that, modify ``feature_test_macros`` table in the script ``utils/generate_feature_test_macro_components.py``, run it, and commit updated files.
+When adding or updating feature-test macros, you should update the corresponding tests.
+To do that, modify ``feature_test_macros`` table in the script
+``utils/generate_feature_test_macro_components.py``, run it, and commit updated
+files. 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.
+
 
 Adding a new header TODO
 ========================
 
 When adding a new header to libc++:
 
 1. Add a test under ``test/libcxx`` that the new header defines ``_LIBCPP_VERSION``. See ``test/libcxx/algorithms/version.pass.cpp`` for an example.
 2. Run ``python utils/generate_header_tests.py``, verify and commit the modifications.
 3. Create a submodule in ``include/module.modulemap`` for the new header.
 4. Update the ``include/CMakeLists.txt`` file to include the new header.
 
 Exporting new symbols from the library
 ======================================
 
-When exporting new symbols from libc++, one must update the ABI lists located in ``lib/abi``.
+When exporting new symbols from libc++, you must update the ABI lists located in ``lib/abi``.
 To test whether the lists are up-to-date, please run the target ``check-cxx-abilist``.
 To regenerate the lists, use the target ``generate-cxx-abilist``.
+The ABI lists must be updated for all supported platforms; currently Linux and
+Apple.  If you don't have access to one of these platforms, you can download an
+updated list from the failed build at
+`Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__.
+Look for the failed build and select the ``artifacts`` tab. There, download the
+abilist for the platform, e.g.:
+
+* C++20 for the Linux platform.
+* MacOS C++20 for the Apple platform.