[libc++][doc] Improves contribution page.

[Mordante]

Summary

This adds more information regarding the libc++ coding style and
reference that are useful when working on a standard library
implementation.

This information is based on review comments and tips I give to new
contributors an information I wish I'd know when I started working on
libc++.

Depends on D156051

[jloser]

So happy to see this finally get written up. I appreciate you doing this! This will be very helpful for new contributors.

[tschuett]

libcxx/docs/Contributing.rst

51

are can't reads odd.

[philnik]

libcxx/docs/Contributing.rst

44

Maybe we should mention that we use clang-tidy for some of these things? I think that is good motivation to enable it in your IDE.

51

53–54

This applies to all types, not just user-provided types. Maybe something like "Unqualified function calls result in ADL"? Also, linking to https://en.cppreference.com/w/cpp/language/adl would be nice, since most people aren't super familiar with ADL.

59–62

I'd just avoid operator, altogether if possible. It makes the code almost always easier to read.

75

147–149

150

https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations would also be quite useful I think.

[Mordante]

Addresses review comments.

[Mordante]

In D156052#4525815, @jloser wrote:

So happy to see this finally get written up. I appreciate you doing this! This will be very helpful for new contributors.

You're welcome. I hope it indeed helps new contributors.

libcxx/docs/Contributing.rst

44

Does clang-tidy work out of the box in IDEs. I've heard some people mention there are issues due to copying our files to the build directory. (I tend to use clang-tidy from the command line.)

53–54

True, but I think the biggest issue is with types not in the std namespace.

59–62

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.

150

Good one, I actually have it in the bookmarks of my browser.

[philnik]

libcxx/docs/Contributing.rst

44

It works just fine for me. I have some custom stuff for parts of my setup to work, but clang-tidy worked from the start. That was the reason for D113849.

53–54

I just want to avoid confusing people. The current wording sound good to me.

59–62

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.

[var-const]

Thanks for writing this! Suggested a few wording tweaks.

libcxx/docs/Contributing.rst

40

Nit: s/In general/In general,/.

42

Nit: s/this standard/these standards/.

44

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.

44

Consider:

The Standard reserves names prefixed with two underscores, or an underscore followed by a capital letter, for implementations, so users...

46

Nit: add a comma after "like `T`".

47

Optional: "may have defined a macro".

48

Nit: this repeats "like all standard libraries", I'd drop this sentence.

49

Nit: add a comma after "clashes".

51

Nit: how about "To avoid some common clashes"?

53

Nit: how about

Unqualified function calls are susceptible

? This problem isn't really specific to the standard library.

54

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.

55

Nit: add a comma after "Therefore".

56

Nit: I'd use a prescriptive, s/are using/must use/.

56

How about:

The exception are some functions in the standard library that require ADL usage.

?

57

Is there a more specific link?

58

Nit: s/subjected/subject/.

62

Nit: consider hyphenating "user-defined".

62

Nit: double space.

63

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 ,.

71

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.

77

Nit: add a comma after "In general".

89

Nit: s/and for bug fixes or features/.

90

Nit: s/in/with/.

91

Nit: add a comma after "In general".

91

Can this be expanded to explain why we might not want purely formatting patches?

103

Nit: add a : or a -- between the file name and "this file contains".

108

I think it would be slightly easier to read if it named the macro here (I'd put the name in parentheses).

129

Nit: I'd capitalize "HTML".

129

Nit: s/A/An/.

136

Nit: add a comma after "features".

[Mordante]

Thanks for the review!

libcxx/docs/Contributing.rst

44

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.

48

I prefer to keep this one since here we mention compiler too. I'll remove it from the first one.

51

I used "To avoid common clashes". To me "some common" sounds odd.

54

Fair point, but I've use the more typical form "argument-dependent lookup (ADL)"

56

I like my wording better.

57

I thought I had changed it to link to p3. Changed it.

59–62

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.

91

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.

108

I think just adding the macro in parentheses doesn't improve readability, instead I changed the sentence to include the macro name.

[Mordante]

Addresses review comments.

[ldionne]

Thanks for working on this!

libcxx/docs/Contributing.rst

49

78–79

87–89

102

107–110

We already have a reference to the Discord channel above, so I'd drop this bullet point.

[var-const]

Thank you for working on this! LGTM with the remaining couple comments applied. I'll leave the final approval to Louis since he also had a few comments.

libcxx/docs/Contributing.rst

44

Nit: either "Libc++ uses `__ugly_names" or "In libc++, we use __ugly_names`" (or similar).

63

Nit: s/Similar/Similarly/.

86

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.

?

101

Nit: this line seems too long.

[Mordante]

Thanks for the reviews!

libcxx/docs/Contributing.rst

86

Fair point, I've applied your suggestion.

101

Correct! I avoided reformatting it until the last review. That makes it easier to see the differences.

I've now fixed this place and other places where the source formatting was off.

[Mordante]

Rebased and addresses review comments.

libcxx/docs/Contributing.rst

 ===========================================
 
 Before you start working on a change that can have significant impact on users of the library,
 please consider creating a RFC on `libc++'s Discourse forum <https://discourse.llvm.org/c/runtimes/libcxx>`__.
 This will ensure that you work in a direction that the project endorses and will ease reviewing your
 contribution as directional questions can be raised early. Including a WIP patch is not mandatory, but
 it can be useful to ground the discussion in something concrete.
 
+Coding standards
+================
+
+In general, libc++ follows the
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_.
+There are some deviations from these standards.
+
+Libc++ uses ``__ugly_names``. These 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 defined 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 common clashes with other uglified names used in
+other implementations (e.g. system headers), the test in
+``libcxx/test/libcxx/system_reserved_names.gen.py`` contains the list of
+reserved names that can't be used.
+
+Unqualified function calls are susceptible to
+`argument-dependent lookup (ADL) <https://en.cppreference.com/w/cpp/language/adl>`_.
+This means calling ``move(UserType)`` might not call ``std::move``. Therefore,
+function calls must use qualified names to avoid ADL. Some functions in the
+standard library `require ADL usage <http://eel.is/c++draft/contents#3>`_.
+Names of classes, variables, concepts, and type aliases are not subject to ADL.
+They don't need to be qualified.
+
+Function overloading also applies to operators. Using ``&user_object`` may call
+a user-defined ``operator&``. Use ``std::addressof`` instead. Similarly, to
+avoid invoking a user-defined ``operator,``, make sure to cast the result to
+``void`` when using the ``,``. For example:
+
+.. code-block:: cpp
+
+    for (; __first1 != __last1; ++__first1, (void)++__first2) {
+      ...
+    }
+
+In general, try to follow the style of existing code. There are a few
+exceptions:
+
+- ``_VSTD::foo`` is no longer used in new code. Use ``std::foo`` instead.
+- ``_LIBCPP_INLINE_VISIBILITY`` is no longer used in new code. Use
+  ``_LIBCPP_HIDE_FROM_ABI`` instead.
+- Prefer ``using foo = int`` over ``typedef int foo``. The compilers supported
+  by libc++ accept alias declarations in all standard modes.
+
+Other tips are:
+
+- Keep the number of formatting changes in patches minimal.
+- Provide separate patches for style fixes and for bug fixes or features. Keep in
+  mind that large formatting patches may cause merge conflicts with other patches
+  under review. In general, we prefer to avoid large reformatting patches.
+- Keep patches self-contained. Large and/or complicated patches are harder to
+  review and take a significant amount of time. It's fine to have multiple
+  patches to implement one feature if the feature can be split into
+  self-contained sub-tasks.
+
+
+Resources
+=========
+
+Libc++ specific
+---------------
+
+- ``libcxx/include/__config`` -- this file contains the commonly used
+  macros in libc++. Libc++ supports all C++ language versions. Newer 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
+  making this available in C++17 or earlier, so we use a macro to implement
+  this requirement.
+- ``libcxx/test/support/test_macros.h`` -- similar to the above, but for the
+  test suite.
+
+
+ISO C++ Standard
+----------------
+
+Libc++ implements the library part of the ISO C++ standard. The official
+publication must be bought from ISO or your national body. This is not
+needed to work on libc++, there are other free resources available.
+
+- The `LaTeX sources <https://github.com/cplusplus/draft>`_  used to
+  create the official C++ standard. This can be used to create your own
+  unofficial build of the standard.
+
+- An `HTML rendered version of the draft <https://eel.is/c++draft/>`_  is
+  available. This is the most commonly used place to look for the
+  wording of the standard.
+
+- An `alternative <https://github.com/timsong-cpp/cppwp>`_ is available.
+  This link has both recent and historic versions of the standard.
+
+- When implementing features, there are
+  `general requirements <https://eel.is/c++draft/#library>`_.
+  Most papers use this
+  `jargon <http://eel.is/c++draft/structure#specifications>`_
+  to describe how library functions work.
+
+- The `WG21 redirect service <https://wg21.link/>`_ is a tool to quickly locate
+  papers, issues, and wording in the standard.
+
+- The `paper trail <https://github.com/cplusplus/papers/issues>`_ of
+  papers is publicly available, including the polls taken. It
+  contains links to the minutes of paper's discussion. Per ISO rules,
+  these minutes are only accessible by members of the C++ committee.
+
+- `Feature-Test Macros and Policies
+  <https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations>`_
+  contains information about feature-test macros in C++.
+  It contains a list with all feature-test macros, their versions, and the paper
+  that introduced them.
+
+- `cppreference <https://en.cppreference.com/w/>`_ is a good resource
+  for the usage of C++ library and language features. It's easier to
+  read than the C++ Standard, but it lacks details needed to properly implement
+  library features.
+
+
 Pre-commit check list
 =====================
 
 Before committing or creating a review, please go through this check-list to make
 sure you don't forget anything:
 
 - Do you have :ref:`tests <testing>` for every public class and/or function you're adding or modifying?
 - Did you update the synopsis of the relevant headers?