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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D77961#2552269 <https://reviews.llvm.org/D77961#2552269>, @cjdb wrote:

>> More generally, have we looked into the MSVC tests for concepts? Would it make sense to import them here too or at least make sure we have the same coverage?
>
> From what I've seen, MSVC seems to have one monolithic file for all the concepts, so I think there are two options for us to choose from if we want to include them:
>
> 1. Wait till all of [concepts] is implemented, then add the MSVC test file at the root.
> 2. Split the monolithic file up so it fits in with all of the libc++ tests.
>
> The advantage of approach (1) is that it's fast, and MSVC STL contributors can painlessly update our copy of the test file whenever theirs is also updated.
> However, it's a monolithic test suite, and I'm not really keen on that: I prefer my tests to be more fine-grained, but this is more work, meaning we'd lose the convenience of getting MSVC STL updates almost for free.
>
> Which do you prefer?

I think we could do (1). In the meantime, let's try to make sure we have good test coverage by ourselves (as you did here).



================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:13
+// template<class From, class To>
+// concept convertible_to;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961