[libcxx][docs] Add section for header layout and guidlines.

[cjdb]

LGTM, possibly add another bullet point saying that tiny elements don't get their own header (e.g. the contents of <type_traits> won't be split into loads of tiny headers with exactly one query or modifier and its shorthand).

libcxx/docs/Contributing.rst

72–74

I think a bit more justification on why this is undesirable is necessary.

[Mordante]

Thanks for documenting this! In general I like it a lot, but I've some suggestions for improvments.

libcxx/docs/Contributing.rst

71

Please add a bullet these headers should have a .h extension.

72–74

+1 it would be nice it this bullet gives a bit more guidance.

78

Maybe add the reason why it's required? For example Thid inclusion is mandatory since the standard requires the header to provide these declarations.

78

Can you add a bullet that the synopsis should be in the parent header and not duplicated in the sub-header?

[• Quuxplusone]

libcxx/docs/Contributing.rst

70–71

Stronger: The current naming convention is exactly __<toplevelheadername>/<featurename>.h. toplevelheadername must be the exact name of whatever header the Standard requires to contain this feature, e.g. string or ranges or memory. featurename should be the exact public identifier of the feature defined in this header, e.g. iterator_traits or construct_at. Exceptions exist (e.g. maybe __iterator/iterator_tags.h; __functional/ops.h) but I expect those exceptions are very rare.

If we enforce the general rule "Name the header after what it defines," then we successfully prevent most attempts to introduce "__detail/utils.h" (because detail is not the name of a top-level header and utils is not a public identifier).

75–78

Someone likes the word "imperative" lately. :)
I suggest: All libc++ headers, both public and helper headers, should "Include What You Use." You should never rely on some other header to transitively include something that you need. If your header needs a definition of iterator_traits, then you should either #include <iterator> (exactly as user code does), or, to improve compile speed, #include <__iterator/iterator_traits.h>. You should never expect iterator_traits to be "pulled in" by any other header.
User code, on the other hand, should never #include any of these helper headers directly; they are intended as unstable implementation details of libc++.

[zoecarver]

• Apply feedback

libcxx/docs/Contributing.rst

75–78

Someone likes the word "imperative" lately. :)

:) sometimes I like a word and it gets stuck in my mind so I use it frequently.

[ldionne]

libcxx/docs/Contributing.rst

75

I don't see a reason to mandate that. I also don't see a reason to go more than one level deep, but I would just not mention that in the policy since it otherwise creates a rule without having a real reason to. I think we can create the rule later if we do have a concrete reason to prohibit multiple levels.

78

I disagree with this rule as it mandates a too high granularity for headers. I think it's entirely reasonable to bundle several public names into the same header if they are related.

An example of that is __memory/shared_ptr.h. It would be crazy to define std::make_shared in a separate header even though it's technically a different public name. The rule should be to try and group things that are logically related together in a way that makes sense, but I don't think we can have a 100% systematic rule here. We'll always have to use our judgement, and that's fine.

80–88

Include-what-you-use is different from requiring that all sub-headers are included by their parent header. I think they should be separate points.

[• Quuxplusone]

libcxx/docs/Contributing.rst

75

I think we can create the rule later if we do have a concrete reason to prohibit multiple levels.

The most likely concrete reason would be, "Someone tried to actually do it." I'd much rather have the rule, and make the prospective-rule-breaker ask to open a discussion about changing the rule; than not have the rule, and permit the prospective-rule-breaker to submit a full-fledged PR for something and then defend it like "well, there's no rule against what I'm doing here." The whole point of D101098 as I understand it is to give us a place to point to and say, yes, there is a rule against that, because people cannot be [trusted and/or assumed] to figure this stuff out by osmosis anymore.

78

FWIW, I agree about shared_ptr+make_shared. I would still like something in writing discouraging the use of "utils" or "helpers" headers.
Perhaps it's useful to say: A header, even an internal one, should be able to state clearly either "what concrete feature it exports" or "its reason for being a separate header." Headers that do not exist for a clearly defined performance reason and do not export a clearly delineated concrete library feature (such as "shared_ptr and friends" or "iterator_traits and its supporting details") probably should not exist.

[Mordante]

I like the shape this patch takes. Before approving I like to see all review comments addressed.

libcxx/docs/Contributing.rst

69

s/whatever/the/, sounds better IMO.

[ldionne]

libcxx/docs/Contributing.rst

75

Let's word it as "We try to keep directories no more than one level deep to keep things simple." That way it's not worded as a hard rule but we still mention it, so you've got your "place to point at" if you want to push back on a review that has abusive usage of nested directories.

78

I agree with that. We want to discourage utility headers that are just a mash-up of "useful things", and this doc should mention it.

libcxx/docs/Contributing.rst

 When adding a new header to libc++:
 
 1. Add a test under ``test/libcxx`` that the new header defines ``_LIBCPP_VERSION``. See ``test/libcxx/algorithms/version.pass.cpp`` for an example.
 2. Run ``python utils/generate_header_tests.py``; verify and commit the changes.
 3. Modify ``python utils/generate_header_inclusion_tests.py``; run it; verify and commit the changes.
 4. Create a submodule in ``include/module.modulemap`` for the new header.
 5. Update the ``include/CMakeLists.txt`` file to include the new header.
 
+Header layout
+=============
+
+Libc++ uses small and contained headers to implement various features. For example, if
+you were going to implement the ``std::ranges::size`` customization point object, you might
+consider creating a new header to house this new feature.
+
+There are a few things to keep in mind when creating new headers:
+
+* The current naming convention is exactly ``__<toplevelheadername>/<featurename>.h``
+  (note the double underscore mangling of the parent directory only and the ``.h`` file extension on the ``<featurename>.h`` header).
+  ``toplevelheadername`` must be the exact name of whatever header the Standard requires
+  to contain this feature, e.g. ``string`` or ``ranges`` or ``memory``. ``featurename``
+  should be the exact public identifier of the feature defined in this header, e.g.
+  ``iterator_traits`` or ``construct_at``. This should be the only feature defined in this
+  header.
+
+    - To clarify: the headers should only go one level deep. This means you could create a
+      header named ``__memory/shared_ptr.h``, but you couldn't create a header
+      ``__memory/smart_pointers/shared_ptr.h``.
+    - To clarify: defining exactly one public identifier per header, and naming the header
+      appropriately forbids headers such as ``__detail/utils.h``.
+* All sub-headers should be included by their parent header. To use the example above,
+  ``<ranges>`` must include the sub-header ``__ranges/size.h``, even if it
+  doesn't use ``ranges::size`` itself. This inclusion is mandatory because the standard
+  requires the top level header to provide these declarations. This rule falls out of the
+  more general "Include What You Use." You should never rely on some other header to
+  transitively include something that you need. If your header needs a definition of
+  ``iterator_traits``, then you should either ``#include <iterator>`` (exactly as user
+  code does), or, to improve compile time, ``#include <__iterator/iterator_traits.h>``.
+  You should never expect ``iterator_traits`` to be "pulled in" by any other header.
+* User code and test code, on the other hand, should never ``#include`` any of these
+  helper headers directly; they are intended as unstable implementation details of libc++.
+* The synopsis comment for a feature should still go in the parent header. Even if the
+  feature isn't implemented in the parent header.
+
 Exporting new symbols from the library
 ======================================
 
 When exporting new symbols from libc++, you must update the ABI lists located in ``lib/abi``.
 To test whether the lists are up-to-date, please run the target ``check-cxx-abilist``.
 To regenerate the lists, use the target ``generate-cxx-abilist``.
 The ABI lists must be updated for all supported platforms; currently Linux and
 Apple.  If you don't have access to one of these platforms, you can download an
 updated list from the failed build at
 `Buildkite <https://buildkite.com/llvm-project/libcxx-ci>`__.
 Look for the failed build and select the ``artifacts`` tab. There, download the
 abilist for the platform, e.g.:
 
 * C++20 for the Linux platform.
 * MacOS C++20 for the Apple platform.