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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM except nitpick about formatting in the tests. No need to wait for my re-review.



================
Comment at: libcxx/include/concepts:195
+  common_reference_with<
+    add_lvalue_reference_t<common_type_t<_Tp, _Up>>,
+    common_reference_t<
----------------
cjdb wrote:
> ldionne wrote:
> > 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.
> > 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?
> 
> This asks the question "does `common_type_t<T, U>&` have a reference in common with `common_reference_t<T const&, U const&>`?" `common_reference_t<T const&, U const&>` should fall out as a reference-to-`const` unless `basic_common_reference` dictates otherwise, so it's essentially checking something to the effect of `common_reference_with<CT&, CR const&>`.
> 
> As for why this is the case, it screams proxy types to me (you wouldn't notice that unless you've played with `zip_view`). This is about as far as my knowledge goes though.
> 
> @CaseyCarter will probably correct me here, but getting stuff wrong on the Internet is how one learns?
I'm still curious to know the answer to this (@CaseyCarter  maybe?), but this shouldn't hold up the review.


================
Comment at: libcxx/test/std/concepts/lang/common.compile.pass.cpp:527-537
+namespace std {
+template <>
+struct common_type<T7, int> {
+  using type = CommonTypeNoMetaCommonReference;
+};
+
+template <>
----------------
I think these specializations would be glance over if you disregarded clang-format and defined them on a single line like

```
...
template <> struct common_type<T7&, int&> { using type = void; };
...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96683