[libcxx-commits] [PATCH] D74291: [libcxx] Adds [concept.same]

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter requested changes to this revision.
CaseyCarter added a comment.
This revision now requires changes to proceed.

Missing a bunch of "Add a new header" changes (https://github.com/llvm/llvm-project/blob/master/libcxx/NOTES.TXT#L19-L29).



================
Comment at: libcxx/include/concepts:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
Shouldn't this line have the same length as line 2?


================
Comment at: libcxx/include/concepts:139
+#include <type_traits>
+
+_LIBCPP_PUSH_MACROS
----------------
#pragma GCC system header? https://github.com/llvm/llvm-project/blob/master/libcxx/include/any#L91-L93


================
Comment at: libcxx/include/concepts:145
+
+#if _LIBCPP_STD_VER > 17
+
----------------
Should this also guard with `defined(__cpp_concepts) && __cpp_concepts >= 201811L`? There are a great many compiler (version)s with C++20 mode that don't implement concepts - I assume libc++ wants to continue supporting them.



================
Comment at: libcxx/include/concepts:149
+template <class T, class U>
+concept __same_as_impl = __is_same(T, U);
+
----------------
miscco wrote:
> miscco wrote:
> > Thinking about it more, this will not work with other compilers such as MSVC. You should seither check the Compiler aka #ifdef __clang__ or the existence of the keyword
> As the code was not displayed properly
> 
> ```
> template <class _Tp, class _Up>
> concept __same_as_impl =
> #ifdef __clang__
>     __is_same(_Tp, _Up);
> #else
>     _VSTD::is_same_v<_Tp, _Up>;
> #endif
> ```
Clang has supported `__is_same` since roughly clang 3. I didn't remember precisely how far back libc++ supports, but it's not that far - so assuming `__is_same` when `__clang__` is fine. GCC since version 6 supports `__is_same_as` with the same semantics. (MSVC has none of the above, but `std::is_same_v` is one of the type traits we recognize and implement directly in the compiler front end, so it's ideal to simply use `is_same_v` with MSVC.)



================
Comment at: libcxx/test/std/concepts/lang/same_as.pass.cpp:81
+      std::same_as<typename Modifier<C3>::type, typename Modifier<C3>::type>);
+  static_assert(std::same_as<typename Modifier<C4<int> >::type,
+                             typename Modifier<C4<int> >::type>);
----------------
I think we can trust a C++20 compiler to implement C++11 correctly and accept `>>` here without the space ;)


================
Comment at: libcxx/test/std/concepts/lang/same_as.pass.cpp:153
+
+int main() {
+  { // Checks std::same_as<T, T> is true
----------------
libc++ tests always use `int main(int, char**) {` (something something freestanding - I don't recall the details)


================
Comment at: libcxx/test/std/concepts/lang/same_as.pass.cpp:189
+    static_assert(!std::same_as<C5<int, double>, C5<double, int> >);
+  }
+}
----------------
There's tons of coverage here for class types and reference types. What about `void`, function types, pointers, arrays, pointers-to-members? (I don't know about clang/libc++, but the MSVC STL tests for type traits tend to _be_ the tests for compiler intrinsics used to implement those traits, so we consequently cover everything under the sun.)


================
Comment at: libcxx/test/std/concepts/lang/same_as.pass.cpp:190
+  }
+}
----------------
libc++ tests also always explicitly `return 0`. (Ditto freestanding yada yada)


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

https://reviews.llvm.org/D74291