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

Fri Apr 30 16:41:45 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/src/CMakeLists.txt:67
   list(APPEND LIBCXX_SOURCES
+    include/locale_sso_allocator.h
     ios.cpp
----------------
ldionne wrote:
> Why is that needed? We normally don't add headers to the list of sources?
I agree that AFAIK from my knowledge of CMake, it should not be needed. However, look at lines 19-21, 32-43, 89-90, 101-104.
I'd be happy to kill off those lines at the same time as this one (in fact I'll update this patch to do so), but if we keep those lines then I think we should keep this one too, because presumably they're all there for the same reason (or non-reason).

I see the comment `Add all the headers to the project for IDEs` on line 118. Is that the rationale for adding these .h files to `LIBCXX_SOURCES`?

================
Comment at: libcxx/src/locale.cpp:38
 #include "include/atomic_support.h"
+#include "include/locale_sso_allocator.h"
 #include "__undef_macros"
----------------
ldionne wrote:
> 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?
Will do. I had put `locale_` in there precisely //because// it's used only by `locale` and I assume the intent was to have it for that one purpose and not "leak" out any more widely than that; but I don't particularly care either way.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101293