[libcxx-commits] [PATCH] D99641: [libcxx] [test] Fix invocable tests on Windows

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In the test suite, we generally try to stay divorced from compiler-specific behavior if we can, the reason being that things otherwise become quite complicated. Furthermore, we're trying to test the library after all, not the language implementation (since there's another test suite for that). I do agree that the boundary between those two things (low-level library stuff and compiler stuff) is sometimes vague, for example when testing type traits that are all implemented as intrinsics.

However, in this instance, it seems to me that we're really trying to test whether the pure library implementation does the right thing. If I had written those tests from the start, I might have gone for something like what Arthur suggested above, which IMO more clearly indicates what we're trying to test:

  struct multiple_overloads {
      struct A {};
      struct B { B(int); };
      struct AB : A, B {};
      struct O {};
      void operator()(A) {};
      void operator()(B) {};
  };
  static_assert(std::invocable<multiple_overloads, multiple_overloads::A>);
  static_assert(std::invocable<multiple_overloads, multiple_overloads::B>);
  static_assert(std::invocable<multiple_overloads, int>);
  static_assert(!std::invocable<multiple_overloads, multiple_overloads::AB>);
  static_assert(!std::invocable<multiple_overloads, multiple_overloads::O>);

On the other hand, I also agree with @cjdb that this test has been useful as-is: it has taught us that MSVC (and Clang in MS mode) doesn't work properly w.r.t. overload resolution. However, I believe the better location for testing that overload resolution bug would be to add (roughly) the same test to the Clang test suite (which it seems is being done). By doing that, we do weaken the libc++ test suite w.r.t. language conformance, but we don't weaken it w.r.t. library conformance.

That's just my .02, but in all cases I would rather not just hide the issue by using `long long`.


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

https://reviews.llvm.org/D99641