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

Wed Aug 2 09:49:14 PDT 2023

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

Thanks for the review!

================
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:
> 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...
I prefer not to specify them since the list is longer any `__` is reserved for the implementation, also names with a leading underscore in the global namespace. For classes we don't always use _CamelCase, I've used __snake_case too.

There is a line like "In general try to follow the style of existing code." which I think covers this. Some headers have a different style than others. I think it would be good to be consistent per header. It would be great when we could make all code uniform, but I don't see that as realistic.

================
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
----------------
var-const wrote:
> Nit: this repeats "like all standard libraries", I'd drop this sentence.
I prefer to keep this one since here we mention compiler too. I'll remove it from the first one.

================
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.
+
----------------
var-const wrote:
> Nit: how about "To avoid some common clashes"?
I used "To avoid common clashes". To me "some common" sounds odd.

================
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
----------------
var-const wrote:
> 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.
Fair point, but I've use the more typical form "argument-dependent lookup (ADL)"

================
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:
> 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.
> ?
I like my wording better.

================
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.
----------------
var-const wrote:
> Is there a more specific link?
I thought I had changed it to link to p3. Changed it.

================
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:
> var-const wrote:
> > Nit: add a comma after "In general".
> Can this be expanded to explain why we might not want purely formatting patches?
I think the most important reason is for the merge conflicts.
I really want to discourage new contributes to have a format everything patch.
I still want to look at how we can slowly get to a state where we have everything clang-formatted.

================
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.
----------------
var-const wrote:
> I think it would be slightly easier to read if it named the macro here (I'd put the name in parentheses).
I think just adding the macro in parentheses doesn't improve readability, instead I changed the sentence to include the macro name.

================
Comment at: libcxx/docs/Contributing.rst:59-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
+result to ``void``. For example:
----------------
philnik wrote:
> Mordante wrote:
> > philnik wrote:
> > > I'd just avoid `operator,` altogether if possible. It makes the code almost always easier to read.
> > We use `operator,` in for loops. So I think it's good to have information why that is done. I've noticed you remove them sometimes. Still this is used in the wording of the Standard so it's good to know why this is done.
> I'm not against explaining why it is done. As-is the text sounds to me like we encourage using `operator,` for these kinds of loops, which at least I definitely do not.
> I'm not against explaining why it is done. As-is the text sounds to me like we encourage using `operator,` for these kinds of loops, which at least I definitely do not.

The intention is not to encourage it, but to explain this is a pitfall. We don't have a clang-tidy check for it so I want to document it here. Personally I'm fine with these kind of loops. At other place I find `operator,` often more questionable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156052