Page MenuHomePhabricator

[libc++] Move <__sso_allocator> out of include/ into src/. NFCI.
ClosedPublic

Authored by Quuxplusone on Apr 26 2021, 7:00 AM.

Details

Summary
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.

Diff Detail

Event Timeline

Quuxplusone created this revision.Apr 26 2021, 7:00 AM
Quuxplusone requested review of this revision.Apr 26 2021, 7:00 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 26 2021, 7:00 AM

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?

Quuxplusone added inline comments.Apr 28 2021, 8:16 AM
libcxx/src/locale.cpp
38

@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.

ldionne added inline comments.Apr 28 2021, 1:40 PM
libcxx/src/locale.cpp
38

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.

Quuxplusone added inline comments.Apr 28 2021, 1:49 PM
libcxx/src/locale.cpp
38

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.

ldionne requested changes to this revision.Apr 30 2021, 2:25 PM
ldionne added inline comments.
libcxx/src/CMakeLists.txt
67

Why is that needed? We normally don't add headers to the list of sources?

libcxx/src/locale.cpp
38

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?

This revision now requires changes to proceed.Apr 30 2021, 2:25 PM

Rename the header file.
Also preemptively remove all non-.cpp files from LIBCPP_SOURCES to see what breaks. I imagine something will break.

Quuxplusone added inline comments.
libcxx/src/CMakeLists.txt
67

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?

libcxx/src/locale.cpp
38

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 added inline comments.
libcxx/src/CMakeLists.txt
67

CMake indeed only needs for the IDE. (Unless the header is generated, but that's not the case here.)

Quuxplusone marked 2 inline comments as done.May 2 2021, 8:35 AM
Quuxplusone added inline comments.
libcxx/src/CMakeLists.txt
67

@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".)

Mordante added inline comments.May 2 2021, 10:00 AM
libcxx/src/CMakeLists.txt
67

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.

Quuxplusone edited the summary of this revision. (Show Details)

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.

ldionne accepted this revision.May 5 2021, 10:43 AM
This revision is now accepted and ready to land.May 5 2021, 10:43 AM