[libcxx-commits] [PATCH] D99290: [libc++] Update contributor documentation.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/docs/Contributing.rst:40
+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.
 
----------------
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."


================
Comment at: libcxx/docs/Contributing.rst:49
+headers           An array with the headers that should provide the
+                  feature-test macro.
+depends           An optional string field. When this field is provided
----------------
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."


================
Comment at: libcxx/docs/Contributing.rst:61
+                  It shall not depend on a macro defined in ``include/__config``.
+internal_depends  An optional string field. When this field is provided
+                  ``depends`` must also be provided. It contains the dependency:
----------------
ldionne wrote:
> 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?
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!


================
Comment at: libcxx/docs/Contributing.rst:68
+                  is only used when a feature isn't fully implemented. Once
+                  fully implemented the field shall be removed.
+================  ==============================================================
----------------
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.


================
Comment at: libcxx/docs/Contributing.rst:89-90
+The ABI lists must be updated all supported platforms; currently Linux and Apple.
+If you don't have access to one of these platforms an updated list can be
+downloaded from the failed build at
+`Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__.
----------------
curdeius wrote:
> 



================
Comment at: libcxx/docs/Contributing.rst:93
+Look for the failed build and select the ``artifacts`` tab. There download the
+abilist for the plafform, e.g.:
+
----------------
curdeius wrote:
> 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99290/new/

https://reviews.llvm.org/D99290