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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/concepts:145
+
+#if _LIBCPP_STD_VER > 17
+
----------------
CaseyCarter wrote:
> 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.
> 
Agreed.


================
Comment at: libcxx/include/concepts:148
+// [concept.same]
+template <class T, class U>
+concept __same_as_impl = __is_same(T, U);
----------------
miscco wrote:
> You are missing the uglification of all namens throughout the code. Mind not in Tests.
> 
> T -> _Tp
Fixed.


================
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>);
----------------
CaseyCarter wrote:
> I think we can trust a C++20 compiler to implement C++11 correctly and accept `>>` here without the space ;)
Agreed. *Stares intensely at clang-format*


================
Comment at: libcxx/test/std/concepts/lang/same_as.pass.cpp:189
+    static_assert(!std::same_as<C5<int, double>, C5<double, int> >);
+  }
+}
----------------
CaseyCarter wrote:
> 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.)
* `void` (/)
* function types (/)
* pointers (/)
* arrays (/)
* pointers-to-members (/)
* pointers-to-member functions (/)


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

https://reviews.llvm.org/D74291