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

Tue Mar 30 11:28:02 PDT 2021

Mordante marked 7 inline comments as done.
Mordante added a comment.

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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99290