[libcxx-commits] [PATCH] D101293: [libc++] Move <__sso_allocator> out of include/ into src/. NFCI.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/src/locale.cpp:38
 #include "include/atomic_support.h"
+#include "include/locale_sso_allocator.h"
 #include "__undef_macros"
----------------
Quuxplusone wrote:
> @ldionne wrote:
> > actually, at some point I just wanted to get rid of the src/include directory for simplicity and move everything to include/. I fully understand that this means we're shipping those headers even if they are technically not needed by end users. But is that really a problem?
> 
> I would very much prefer to have the directory structure match the intended usage. If we don't //intend// for end-users to include these headers, then they should not be //permitted// to include them (i.e., we should not ship them in the libc++ include path).  Status quo is that the `include/` directory is for libc++ headers, not for arbitrary other headers that happen to be required by the build process of arbitrary llvm-project artifacts (even if that llvm-project artifact is as near a miss as "the libc++.so library itself").
> 
> The main reason I want this file out of the `include/` directory is so that the code's structure will match the code's intent. This is not a libc++ header; we don't want it shipped or installed; we don't want any other `include/` header to ever depend on it; we don't want it to show up as a root in the dependency graph; we don't want it to undergo //any// of the things that normally happen to code in the `include/` directory. So it shouldn't be there.
> 
> (That said, I would also be totally OK if you asked to revise this PR to eliminate the .h file entirely; its code could be placed directly into `src/locale.cpp`. That would be the code-review equivalent of the Google v. Oracle "fair use" decision ;) — it would be quite defensible, while deliberately omitting to either endorse the status-quo "Arthur's" way or set a competing precedent for your proposed "other" way.)
> 
> The idea of "have a defined public API / don't ship what your users can't use" is just generally a core belief of mine. But also, it reminds me of when uthash went through a similar process in https://github.com/troydhanson/uthash/pull/117 — having a bunch of "public but please-don't-use-this-it's-not-for-you" source code was a huge headache for the people making distributions. In libc++'s case obviously the problem is not as problematic and the solution is not as drastic; but still, we //have// a status-quo solution; we should use it.
There are many headers that we don't want users including by themselves - basically anything that isn't a public header described in the standard library. We "enforce" that through naming. The benefit to putting all includes under `include/` is that it's a simple model and it's what most libraries I've seen do.

I think our source of disagreement is that you value the "cleanliness" of minimizing what we ship/expose to users beyond the simplicity of the library itself, whereas for me it's the other way around.

Also, while eradicating the separate file altogether is clever to avoid creating a precedent, I don't think it would improve the code, so no.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101293