Details
- Reviewers
• Quuxplusone Mordante var-const ldionne EricWF - Group Reviewers
Restricted Project - Commits
- rG52915d78f44b: [libc++] Granularize <utility> includes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/vector | ||
---|---|---|
311 | As in the previous PR, please do the removal of these lines in a separate commit (and optionally in a whole separate PR). Because adding #include <__utility/move.h> probably can't break anything, and adding #include <utility> to some tests can't break anything, but removing #include <utility> from existing headers like <vector> and <typeindex> can definitely break users, and if they complain we might want to git revert just that one part. So it should be isolated in its own commit. Actually, looking at your changes in <variant>, it seems like you intended to do what I'm saying here, and this diff just slipped through the cracks? I think what you did in <variant>, adding a line #include <utility> // TODO: Remove this, is good. | |
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.pass.cpp | ||
18–19 ↗ | (On Diff #411045) | This is an interesting case. @var-const, this was your test originally, right? Does including <tuple> here nerf the point of this test? IIRC, you wanted the point of the test to be
Whereas I'm personally happy with scaling that back to
So I would personally approve of adding #include <tuple> here, and then also remove the comment on lines 20-21. But if the point of the test is the first bullet above, then adding new #includes to this file seems like a bad idea. |
libcxx/include/vector | ||
---|---|---|
311 | +1 |
I know I'm late to the party on this, but is there a document explaining the benefits of modularizing the headers? Cuz I see a bunch of downsides to it. The most recent of which I ran into was diagnostic error messages. I'm almost certain it's a lot slower to hit the file system this many times too. It also causes the compiler to emit incorrect Fix i suggestions?
If there isn't a document describing this, we need one.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.pass.cpp | ||
---|---|---|
18–19 ↗ | (On Diff #411045) | Thanks for the heads-up! I think this file should be left unchanged. Adding this include is unnecessary, defeats (a part of) the purpose of the test and doesn't seem to be related to the main intention of this patch.
If I understand the Standard correctly, it explicitly says that including <ranges> should be enough for this case. It can be argued that user code might not want to rely on this subtle detail, but nevertheless it should work. |
Eric, we've been doing this for roughly 1 year now. Arthur has already explained some of the reasons to you, like reducing/removing circular dependencies and being easier to understand (now you don't have to wonder why in the world is std::swap defined in <type_traits>. As a side benefit, it also makes our headers include fewer things transitively, which enforces slightly better IWYU for users.
Regarding compile-times: the right way to speed things up is to use modules and have more minimal headers, not to try to reduce the number of filesystem accesses by reducing the number of files.
Regarding diagnostics, I'm not sure how they are really made worse by this. If we wanted, we could also piggy-back on the include_instead pragma to tweak error messages. Clang fix-it should definitely listen to include_instead, at the very least.
Cool. I'm suprised we don't have a document describing why we're doing it. And I would like one. I was unaware of circular dependencies prior to this change. And I personally don't find it easier to understand. The "side benefit" of including fewer things will break our users, and at Google IWYU has been adding includes for internal headers (which is not good).
Regarding compile-times: the right way to speed things up is to use modules and have more minimal headers, not to try to reduce the number of filesystem accesses by reducing the number of files.
The right way to speed up compile times is the way that speeds up compile times. Measurement is key.
Regarding diagnostics, I'm not sure how they are really made worse by this. If we wanted, we could also piggy-back on the include_instead pragma to tweak error messages. Clang fix-it should definitely listen to include_instead, at the very least.
When the compiler tells you to include the wrong header, or diagnostics expose implementation details (and hide what user-visible component is responsible for the breakage); it's not good for our users.
I'm not interested in litigating this. I am interested in documenting the rational for doing it. With concrete examples of how it makes the code better. I'm going to stand firm on this. We NEED to document large design decisions like this.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.pass.cpp | ||
---|---|---|
20 ↗ | (On Diff #412417) | Looks like this line needs to be restored as well. |
LGTM when CI passes. I think everybody's comments have been addressed now, except for @EricWF 's request for a design document. I spoke with Eric offline and explained why we were doing this, and why we didn't have a design document (TLDR we had Discord discussions about this and all ended up agreeing on the direction, but we didn't record it formally in a design doc). I think a design doc explaining the rationale for this change is a fair ask, however we can address it separately and I don't think this specific patch should be blocked on that task.
As in the previous PR, please do the removal of these lines in a separate commit (and optionally in a whole separate PR). Because adding #include <__utility/move.h> probably can't break anything, and adding #include <utility> to some tests can't break anything, but removing #include <utility> from existing headers like <vector> and <typeindex> can definitely break users, and if they complain we might want to git revert just that one part. So it should be isolated in its own commit.
Actually, looking at your changes in <variant>, it seems like you intended to do what I'm saying here, and this diff just slipped through the cracks? I think what you did in <variant>, adding a line #include <utility> // TODO: Remove this, is good.