[libcxx-commits] [PATCH] D96683: [libcxx] adds concept `std::common_with`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/concepts:195
+  common_reference_with<
+    add_lvalue_reference_t<common_type_t<_Tp, _Up>>,
+    common_reference_t<
----------------
I see this follows the spec closely, but I'm curious to understand. Here, I would have expected the check to be:

```
common_reference_with<
    common_type_t<_Tp, _Up> const&,
    common_reference_t<
        const _Tp&,
        const _Up&>>;
```

instead (with `add_lvalue_reference_t` simplified naively for legibility). In other words, why are we looking for a common reference between the non-const common type and the common-ref between the `const`-qualified types? I'm sure that'll highlight a misunderstanding I have about the concept.


================
Comment at: libcxx/test/std/concepts/lang/common.compile.pass.cpp:19
+constexpr bool CheckCommonWith() noexcept {
+  static_assert(std::common_with<T, U&>);
+  static_assert(std::common_with<T, const U&>);
----------------
Same comment about testing with `static_assert(std::common_with<T, U&> == std::common_with<T, U>)` as in the other patch, if you think it makes sense.


================
Comment at: libcxx/test/std/concepts/lang/common.compile.pass.cpp:257
+struct BadBasicCommonType {
+  // This test is ill-formed, NDR. If it ever blows up in our faces: that's a good thing.
+  // In the meantime, the test should be included. If compiler support is added, then an include guard
----------------
I'm not sure I follow this. Can you explain?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96683