[libcxx-commits] [PATCH] D156052: [libc++][doc] Improves contribution page.

Thu Aug 24 15:01:12 PDT 2023

var-const accepted this revision as: var-const.
var-const added a comment.

LGTM with the remaining couple comments applied. I'll leave the final approval to Louis since he also had a few comments.

================
Comment at: libcxx/docs/Contributing.rst:44
+
+In libc++, uses ``__ugly_names``. These
+names are reserved for implementations, so users may not use them in
----------------
Nit: either "Libc++ uses ``__ugly_names``" or "In libc++, we use ``__ugly_names``" (or similar).

================
Comment at: libcxx/docs/Contributing.rst:63
+may call a user-defined ``operator&``. Use ``std::addressof`` instead.
+Similar, to avoid invoking a user-defined ``operator,``, make sure to cast the
+result to ``void`` when using the ``,``. For example:
----------------
Nit: `s/Similar/Similarly/`.

================
Comment at: libcxx/docs/Contributing.rst:86
+  mind that large formatting patches may cause merge conflicts with other patches
+  under review. In general, we do not want these patches.
+- Keep patches small and self-contained. Large patches are harder to review and
----------------
Sorry for nitpicking, but this sentence reads unintentionally negative to me, can we rephrase it slightly? It's also not 100% clear which patches it refers to (I presume the large formatting patches, but I could be wrong). Can we do something like:
```
In general, we prefer to avoid large reformatting patches.
```
?

================
Comment at: libcxx/docs/Contributing.rst:101
+  versions of the Standard add new features. For example, making
+  functions ``constexpr`` in C++20 is done by using ``_LIBCPP_CONSTEXPR_SINCE_CXX20``. This means the function is
+  ``constexpr`` in C++20 and later. The Standard does not allow to make
----------------
Nit: this line seems too long.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156052