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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961