[libcxx-commits] [PATCH] D124755: [libc++] Granularize parts of <type_traits>

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D124755#3535200 <https://reviews.llvm.org/D124755#3535200>, @philnik wrote:

>> - I don't think this improves the quality of the code,
>
> Not sure what quality you are talking about, but I think the readability is increased a lot. You instantly see what is part of the implementation.

I've never had a problem with that.

>> - It makes our module maps more complicated,
>
> Eh. I wouldn't consider a list of files "complicated".

Fair.

>> - It creates less understandable diagnostics by exposing what should be private implementation details.
>
> Could you give an example?

When Clang produces a diagnostic about a missing declaration when using clang modules, it tells the user to include the private header. And users listen. Every week the number of instances of "#include <__private/foo>" in googles codebase grows as a result.
I can provide other examples as well.

>> - I don't know hat it actually improves portability much; We are the only standard library to do this (at least to this extent), with the claim that exposing more names than necessary creates portability problems, but I don't think that's true for a number of reasons, one of which being that every other standard library includes all of <X> if they need one thing from <X>.
>
> It improves the portability in the way that you are more likely to properly include all the headers you need.

Not if every STL implementation provides the same transitive includes. Also, it only improves portability one way. That is code written against libc++ is likely to port to other STL's without needing additional includes. The other direction is less true. Though I agree that decreasing our transitive includes is a good goal, I don't know if it actually solves a problem our users have in practice, or if it creates a new problem they didn't have before.

I ported all of Google's C++ codebase from libstdc++ to libc++, which is to say "I ported a lot of code", and in my experience, header hygiene didn't cause an major issue. Though it likely will be as we remove transitive declarations.

>> Have we done tests to see how many fewer names we actually expose to the end user for a typical real TU? I'm imaging we could do something like:
>>
>> 1. Produce a pre-processed header.
>> 2. Run clang-query over it to produce a list of STL declarations.
>> 3. Repeat for both HEAD and older versions.
>> 4. Diff the outputs.
>
> I'm not aware of any test like this. I think the outcome would be interesting, but I doubt it will influence the direction very much.

If we're not actually decreasing the surface area  of our headers, which is the primary stated goal, it should influence the direction.

>> Further, as the complexity of our header structure increases, I find it increasingly difficult to reason about the header structure. I'm glad we have a tool to detect circular deps, because that's beyond my capabilities now.  But how are we ensuring all necessary declarations have been included *directly*, rather than transitively. AKA how do we enforce "include what you use"?
>
> That's enforced very well by the modules build when the headers are granularized.

Perfect. Don't know why I didn't think of that.

>> And finally, given the goal of this is to remove unnecessary declarations leaking from headers, how are we tracking our progress doing this? We should be aware exactly what breaking changes we're shipping to users, and I don't know that we are.
>
> We have a few patches that remove top-level headers. The removed headers are listed under https://libcxx.llvm.org/ReleaseNotes.html#id4.

Cool, but do we know the actual effect of that? It's one thing to say "Removed #include <utility> from <foo>", but that doesn't tell us what declarations are no longer present as a result. Which is important. Did we remove 15 declarations or 150?
I have no problem making breaking changes. I do have a problem with doing it in the dark.

>> P.S. Also, and I have been meaning to confirm this, but I expect this to be some amount slower for non-trivial real-world C++ builds. But only when there is enough code being compiled to frequently eject the STL headers from the linux kernels file cache. That said, until I actually produce a test, it's fair to say I'm only talking out my butt.
>
> Ever tried `-fmodules`? Makes the libc++ tests run at least 1.6x as fast on my machine.

Yes, I have tried modules. I've also tried porting large codebases over to modules, and it's non-trivial. So long as a significant portion of our users rely on textual includes, the build time of texually including libc++ matters. But again, until I produce data demonstrating an actual problem, we can assume I'm way off the mark.

> I'm responding here, but I don't want this to block this PR. Could you ask on Discord if you want further clarification/justification?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124755