[libcxx-commits] [PATCH] D77961: [libcxx] adds concept `std::convertible_to`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:13
+// template<class From, class To>
+// concept convertible_to;
+
----------------
cjdb wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > zoecarver wrote:
> > > > > All tests should include "test_macros.h"
> > > > Technically, only include `test_macros.h` if you need something defined in it. The one header we always want to include is the one with nasty macros in it, but it's automatically included by the test suite harness.
> > > I disagree, I think all tests should include this header, regardless of whether or not they actually use it. The most important reason is that sometimes someone will update a test in a few years, and add a condition with `TEST_STD_VER`, not realizing that `TEST_STD_VER` is undefined. For example, before [[ https://github.com/llvm/llvm-project/commit/7fc6a55688c816f5fc1a5481ae7af25be7500356 | 7fc6a55688c816f5fc1a5481ae7af25be7500356 ]] landed, over 300 tests used `TEST_STD_VER` //without// importing `test_macros.h` (not to mention the 3000 others that didn't include `test_macros.h` without references to that macro). This means that some parts of 300 tests were silently and unintentionally not being tested.
> > > 
> > > A less compelling reason that we might want all tests to include this header is to allow portability and more complicated nasty macro testing. For example, I think some people use this test suite without lit, meaning nasty macros won't work for them. Or maybe they don't have a compiler feature that we rely on. I think having an access point into //all// of the tests can be super benifitial in these cases.
> > Don't we get a warning turned into an error for using `#if TEST_STD_VER > XX` if `TEST_STD_VER` isn't defined? That would be my expectation.
> > 
> > I don't think I follow the logic of the argument you make, because in the same spirit, we should always be including all other headers that define any macro to make sure we don't use macros when they're undefined (and have things behave unexpectedly for hard to understand reasons). I really think it's reasonable to not include `test_macros.h` when it's not needed: it makes the test suite more minimal. I think I fixed the issue of silently using an undefined macro when I enabled more warnings by default last year during my work on Lit, but I could be wrong.
> `std::convertible_to` is a blocker for other concepts (e.g. `move_constructible`, `common_reference_with`, etc.), so I'd like to get it in ASAP.
> Is it possible for this to be merged while you debate the need for including `test_macros.h`? I won't include it by default, but once you've reached consensus, I don't mind going back and adding it to all the concepts in one fell swoop (which I'll need to do for the already merged concepts anyway).
It was never my intention to block you on adding or not adding that include :-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961