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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
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
----------------
Quuxplusone wrote:
> 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.
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 the library user intends to refer to `std::tuple_element` by name, they should `#include <tuple>`.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120466