[libcxx-commits] [PATCH] D101098: [libcxx][docs] Add section for header layout and guidlines.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Contributing.rst:75
+
+    - 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
----------------
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.


================
Comment at: libcxx/docs/Contributing.rst:78
+      ``__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``.
----------------
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.


================
Comment at: libcxx/docs/Contributing.rst:80-88
+* 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101098