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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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
----------------
ldionne wrote:
> 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.
> 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.


================
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``.
----------------
ldionne wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101098