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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D99290#2650799 <https://reviews.llvm.org/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!



================
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.
 
----------------
Quuxplusone wrote:
> 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."
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.


================
Comment at: libcxx/docs/Contributing.rst:46
+name              The name of the feature-test macro.
+values            A list of tuples. One tuple contains a C++ version and the
+                  value of the feature-test marco for that C++ version.
----------------
curdeius wrote:
> Isn't it a dictionary (or however it's called in pythonesque)?
Yes, will change it.


================
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
----------------
Quuxplusone wrote:
> 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."
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?


================
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:
----------------
Quuxplusone wrote:
> 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!
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.


================
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.
+================  ==============================================================
----------------
Quuxplusone wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99290