[libcxx-commits] [PATCH] D119439: [libc++] Remove __functional_base

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Feb 11 10:03:15 PST 2022


ldionne added inline comments.


================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/iterator/minus.pass.cpp:20
+#include <cstdint>
+#include <ranges>
 
----------------
philnik wrote:
> ldionne wrote:
> > philnik wrote:
> > > Quuxplusone wrote:
> > > > Please leave `<ranges>` first; historically this is our style for tests that test a particular header (although I admit I don't see much point to it).
> > > I didn't know that rule and nobody has said anything in my other PRs until now, so I'd like to know @ldionne's opinion on this. I don't see much point to it either.
> > That's true, we've always done this (rather blindly TBH). We may not have noticed on your other PRs.
> > 
> > The benefit of having the header first is that we make sure the header can be included when nothing else has been included before. I'm fine with not doing this anymore in our tests, however I think it would be nice to also have generated tests that simply include every single public header in an otherwise empty translation unit, just to make sure it works.
> > 
> > TLDR: Let's test this properly and then we can safely drop this weird custom we have. I'm fine with this patch not following the usual rule.
> You mean something like `test/libcxx/inclusions`? (and extend it to generate tests for all headers)
Yes, exactly, but I wouldn't necessarily extend `generate_header_inclusion_tests.py`, I would just create another one that includes literally every single header, regardless of whether they have mandated transitive includes or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119439



More information about the libcxx-commits mailing list