This allocator is not intended for libc++'s users to use; it's strictly an implementation detail of `src/locale.cpp`. So, move it to the `src/include/` directory. Drive-by const-qualify its comparison operators. For consistency with `__hidden_allocator` (defined in `src/thread.cpp`), do *not* remove it from "libcxx/lib/libc++unexp.exp", "libcxx/utils/symcheck-blacklists/linux_blacklist.txt", etc.
- Group Reviewers
- rG0b10bb7ddd3c: [libc++] Move <__sso_allocator> out of include/ into src/. NFCI.
So 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? Concretely, the size increase in the distribution is in the noise (we might as well get rid of our long synopses). Thoughts?
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.
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.
Why is that needed? We normally don't add headers to the list of sources?
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?
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 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?
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.
@Mordante: "CMake indeed only needs for the IDE" — So, would you say we should keep all the files in CMakeLists.txt for the benefit of IDE-users? Your choice of the word "...only..." is throwing me a little bit. :)
I.e. is this part of the current patch (removing 20 lines from this file) a good idea or a bad idea? (I'm conservatively leaning toward "bad idea".)
As for the "only". CMake doesn't need the header to figure out the dependencies of the .cpp files, but CMake can't determine which headers should be part of your IDE project. You don't want every header (indirectly) included to be part of your project.
Having the headers in the list doesn't hurt us. So I think removing them is a bad idea, it hurts users using an IDE.
Restore .h files to CMakeLists.txt, for the benefit of IDEs.
Drive-by const-qualify its comparison operators (since one of my greps turned up these lines and I noticed they were subpar).
Update summary to match my intended commit message.