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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/src/locale.cpp:38
 #include "include/atomic_support.h"
+#include "include/locale_sso_allocator.h"
 #include "__undef_macros"
----------------
@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101293