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

Fri Apr 30 14:25:50 PDT 2021

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

================
Comment at: libcxx/src/CMakeLists.txt:67
   list(APPEND LIBCXX_SOURCES
+    include/locale_sso_allocator.h
     ios.cpp
----------------
Why is that needed? We normally don't add headers to the list of sources?

================
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:
> > 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.
> > 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.
> 
> Not exactly. I mean, in this case, `__sso_allocator` is **not a libc++ header.** It's a piece of the src/ code; it belongs with the rest of src/.  You wouldn't agitate for moving `src/debug.cpp` out of src/ into include/, right? So there's **some** distinction we're both making between "source code of the shared object" and "public-facing headers of libc++ itself". I'm just including `__sso_allocator` in the former set, rather than the latter set.
> 
> And while this is //partly// about how "we don't want it shipped or installed," it's also about how "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 [files] in the include/ directory."  Right now it's hitting all of those things, and we actually want it to hit none of them.
OK. I'm not swayed by your arguments, but let's do it, it'll make the distribution smaller.

Can you rename it to `sso_allocator.h` though, without tying it to `locale` too specifically?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101293