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

[Louis Dionne via Phabricator via libcxx-commits]

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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101098