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

Tue Aug 1 14:56:45 PDT 2023

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Thanks for writing this! Suggested a few wording tweaks.

================
Comment at: libcxx/docs/Contributing.rst:40
+
+In general libc++ follows the
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_.
----------------
Nit: `s/In general/In general,/`.

================
Comment at: libcxx/docs/Contributing.rst:42
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_.
+There are some deviations from this standard.
+
----------------
Nit: `s/this standard/these standards/`.

================
Comment at: libcxx/docs/Contributing.rst:44
+
+In libc++, like all standard libraries, we use ``__ugly_names``. These
+names are reserved for implementations, so users may not use them in
----------------
Nit: we also use `_UglyNames`. How about:
> In libc++ (like in all other standard library implementations), we have to use "uglified" names for variables (`__some_variable`), classes (`_SomeClass`) and so on.

================
Comment at: libcxx/docs/Contributing.rst:44
+
+In libc++, like all standard libraries, we use ``__ugly_names``. These
+names are reserved for implementations, so users may not use them in
----------------
var-const wrote:
> Nit: we also use `_UglyNames`. How about:
> > In libc++ (like in all other standard library implementations), we have to use "uglified" names for variables (`__some_variable`), classes (`_SomeClass`) and so on.
Consider:
> The Standard reserves names prefixed with two underscores, or an underscore followed by a capital letter, for implementations, so users...

================
Comment at: libcxx/docs/Contributing.rst:46
+names are reserved for implementations, so users may not use them in
+their own applications. When using a name like ``T`` a user
+may have a macro that changes the meaning of ``T``. By using
----------------
Nit: add a comma after "like ``T``".

================
Comment at: libcxx/docs/Contributing.rst:47
+their own applications. When using a name like ``T`` a user
+may have a macro that changes the meaning of ``T``. By using
+``__ugly_names`` we avoid that problem. Other standard libraries and
----------------
Optional: "may have defined a macro".

================
Comment at: libcxx/docs/Contributing.rst:48
+may have a macro that changes the meaning of ``T``. By using
+``__ugly_names`` we avoid that problem. Other standard libraries and
+compilers use these names too. To avoid clashes the test in
----------------
Nit: this repeats "like all standard libraries", I'd drop this sentence.

================
Comment at: libcxx/docs/Contributing.rst:49
+``__ugly_names`` we avoid that problem. Other standard libraries and
+compilers use these names too. To avoid clashes the test in
+``libcxx/test/libcxx/system_reserved_names.gen.py``
----------------
Nit: add a comma after "clashes".

================
Comment at: libcxx/docs/Contributing.rst:51
+``libcxx/test/libcxx/system_reserved_names.gen.py``
+contains the list of reserved names that can't be used.
+
----------------
Nit: how about "To avoid some common clashes"?

================
Comment at: libcxx/docs/Contributing.rst:53
+
+Function calls in the standard library are susceptible to
+`ADL <https://en.cppreference.com/w/cpp/language/adl>`_.
----------------
Nit: how about
> Unqualified function calls are susceptible
? This problem isn't really specific to the standard library.

================
Comment at: libcxx/docs/Contributing.rst:54
+Function calls in the standard library are susceptible to
+`ADL <https://en.cppreference.com/w/cpp/language/adl>`_.
+This means calling ``move(UserType)`` might not call ``std::move``. Therefore
----------------
Nit: how about `ADL (argument-dependent lookup)`? Readers who already know what "ADL" is probably don't need the link, and those who encounter the term for the first time would probably appreciate the long form.

================
Comment at: libcxx/docs/Contributing.rst:55
+`ADL <https://en.cppreference.com/w/cpp/language/adl>`_.
+This means calling ``move(UserType)`` might not call ``std::move``. Therefore
+function calls are using qualified names to avoid ADL. Some functions in the
----------------
Nit: add a comma after "Therefore".

================
Comment at: libcxx/docs/Contributing.rst:56
+This means calling ``move(UserType)`` might not call ``std::move``. Therefore
+function calls are using qualified names to avoid ADL. Some functions in the
+standard library `require ADL usage <http://eel.is/c++draft/contents>`_.  Names
----------------
Nit: I'd use a prescriptive, `s/are using/must use/`.

================
Comment at: libcxx/docs/Contributing.rst:56
+This means calling ``move(UserType)`` might not call ``std::move``. Therefore
+function calls are using qualified names to avoid ADL. Some functions in the
+standard library `require ADL usage <http://eel.is/c++draft/contents>`_.  Names
----------------
var-const wrote:
> Nit: I'd use a prescriptive, `s/are using/must use/`.
How about:
> The exception are some functions in the standard library that require ADL usage.
?

================
Comment at: libcxx/docs/Contributing.rst:57
+function calls are using qualified names to avoid ADL. Some functions in the
+standard library `require ADL usage <http://eel.is/c++draft/contents>`_.  Names
+of classes, variables, concepts, and type aliases are not subjected to ADL.
----------------
Is there a more specific link?

================
Comment at: libcxx/docs/Contributing.rst:58
+standard library `require ADL usage <http://eel.is/c++draft/contents>`_.  Names
+of classes, variables, concepts, and type aliases are not subjected to ADL.
+They don't need to be qualified.
----------------
Nit: `s/subjected/subject/`.

================
Comment at: libcxx/docs/Contributing.rst:62
+Function overloading also applies to operators. Using ``&user_object``
+may call a user defined ``operator&``. Use ``std::addressof``  instead.
+Similar for ``operator,``. When using the ``,`` make sure to cast the
----------------
Nit: consider hyphenating "user-defined".

================
Comment at: libcxx/docs/Contributing.rst:62
+Function overloading also applies to operators. Using ``&user_object``
+may call a user defined ``operator&``. Use ``std::addressof``  instead.
+Similar for ``operator,``. When using the ``,`` make sure to cast the
----------------
var-const wrote:
> Nit: consider hyphenating "user-defined".
Nit: double space.

================
Comment at: libcxx/docs/Contributing.rst:63
+may call a user defined ``operator&``. Use ``std::addressof``  instead.
+Similar for ``operator,``. When using the ``,`` make sure to cast the
+result to ``void``. For example:
----------------
Nit: this sentence is very short, consider combining with the next one:
> Similarly, to avoid invoking a user-defined `operator,`, make sure to cast the result to `void` when using the `,`.

================
Comment at: libcxx/docs/Contributing.rst:71
+      _InputIterator1 __first1, _InputIterator1 __last1, _InputIterator2 __first2, _BinaryPredicate& __pred) {
+    for (; __first1 != __last1; ++__first1, (void)++__first2)
+      if (!__pred(*__first1, *__first2))
----------------
I would leave only this line, otherwise this snippet has many unrelated non-obvious things (e.g. `_LIBCPP_NODISCARD`, `_LIBCPP_HIDE_FROM_ABI`) that make it harder to focus on the essence of the example.

================
Comment at: libcxx/docs/Contributing.rst:77
+
+In general try to follow the style of existing code. There are a few
+exceptions:
----------------
Nit: add a comma after "In general".

================
Comment at: libcxx/docs/Contributing.rst:89
+- Keep the number of formatting changes in patches minimal.
+- Provide separate patches for style fixes and bug-fix or feature. Keep in
+  mind that large formatting patches may cause merge conflicts in other patches
----------------
Nit: `s/and for bug fixes or features/`.

================
Comment at: libcxx/docs/Contributing.rst:90
+- Provide separate patches for style fixes and bug-fix or feature. Keep in
+  mind that large formatting patches may cause merge conflicts in other patches
+  under review. In general we do not want these patches.
----------------
Nit: `s/in/with/`.

================
Comment at: libcxx/docs/Contributing.rst:91
+  mind that large formatting patches may cause merge conflicts in 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
----------------
Nit: add a comma after "In general".

================
Comment at: libcxx/docs/Contributing.rst:91
+  mind that large formatting patches may cause merge conflicts in 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
----------------
var-const wrote:
> Nit: add a comma after "In general".
Can this be expanded to explain why we might not want purely formatting patches?

================
Comment at: libcxx/docs/Contributing.rst:103
+
+- ``libcxx/include/__config`` this file contains the commonly used
+  macros in libc++. Libc++ supports all C++ language versions. Newer
----------------
Nit: add a `:` or a `--` between the file name and "this file contains".

================
Comment at: libcxx/docs/Contributing.rst:108
+  ``constexpr`` in C++20 and later. The Standard does not allow to make
+  this available in C++17 or earlier, so we use a macro to implement this
+  requirement.
----------------
I think it would be slightly easier to read if it named the macro here (I'd put the name in parentheses).

================
Comment at: libcxx/docs/Contributing.rst:129
+
+- A `html rendered version of the draft <https://eel.is/c++draft/>`_  is
+  available. This is the most commonly used place to look for the
----------------
Nit: I'd capitalize "HTML".

================
Comment at: libcxx/docs/Contributing.rst:129
+
+- A `html rendered version of the draft <https://eel.is/c++draft/>`_  is
+  available. This is the most commonly used place to look for the
----------------
var-const wrote:
> Nit: I'd capitalize "HTML".
Nit: `s/A/An/`.

================
Comment at: libcxx/docs/Contributing.rst:136
+
+- When implementing features there are
+  `general requirements <https://eel.is/c++draft/#library>`_.
----------------
Nit: add a comma after "features".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156052