[libcxx-commits] [PATCH] D120466: [libc++] Granularize <utility> includes

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/vector:311
 #include <typeinfo>
-#include <utility>
 
----------------
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.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.ranges.pass.cpp:18-19
 
 #include <ranges>
+#include <tuple>
 // Note: make sure to not include `<utility>` (or any other header including `<utility>`) because it also makes some
----------------
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

- Including `<ranges>` makes a declaration of `std::tuple_element` available.

Whereas I'm personally happy with scaling that back to

- Including `<ranges>` makes structured binding work. (If the library user intends to refer to `std::tuple_element` by name, they should `#include <tuple>`.)

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 `#include`s to this file seems like a bad idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120466