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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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



================
Comment at: libcxx/docs/Contributing.rst:71
+* All sub-directories should be mangled with a double underscore. Conversely, headers
+  that live in these directories should not be mangled.
+* Headers should contain specific features. A header such as ``__detail/utils.h`` might
----------------
Please add a bullet these headers should have a `.h` extension.


================
Comment at: libcxx/docs/Contributing.rst:72-74
+* Headers should contain specific features. A header such as ``__detail/utils.h`` might
+  turn into a dumping ground for "random" utilities. This is counterproductive and actually
+  makes things harder to find.
----------------
cjdb wrote:
> I think a bit more justification on why this is undesirable is necessary.
+1 it would be nice it this bullet gives a bit more guidance.


================
Comment at: libcxx/docs/Contributing.rst:78
+  another example, ``<iterator>`` must include ``__iterator/iterator_traits.h``, even if it
+  doesn't use ``iterator_traits`` itself.
+
----------------
Maybe add the reason why it's required? For example `Thid inclusion is mandatory since the  standard requires the header to provide these declarations.`


================
Comment at: libcxx/docs/Contributing.rst:78
+  another example, ``<iterator>`` must include ``__iterator/iterator_traits.h``, even if it
+  doesn't use ``iterator_traits`` itself.
+
----------------
Mordante wrote:
> Maybe add the reason why it's required? For example `Thid inclusion is mandatory since the  standard requires the header to provide these declarations.`
Can you add a bullet that the synopsis should be in the parent header and not duplicated in the sub-header?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101098