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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added a comment.

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

> - It really muddies the blame history (which has been really really really problematic for me recently,)

That's not ideal, I agree.

> - It makes our module maps more complicated,

Eh. I wouldn't consider a list of files "complicated".

> - It creates less understandable diagnostics by exposing what should be private implementation details.

Could you give an example?

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

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

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

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

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

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?



================
Comment at: libcxx/include/module.modulemap:987
+    module conditional                { private header "__type_traits/conditional.h" }
+    module decay                      { private header "__type_traits/decay.h" }
+    module enable_if                  { private header "__type_traits/enable_if.h" }
----------------
EricWF wrote:
> Should these have non-reserved names?
They should be identical to the header name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124755