[PATCH] D98160: [clang] Use decltype((E)) for compound requirement type constraint

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I like this approach.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2761
 def note_expr_requirement_constraints_not_satisfied_simple : Note<
-  "%select{and|because}0 %1 does not satisfy %2:">;
+  "%select{and|because}0 'decltype((%1))' (aka %2) does not satisfy %3:">;
 def note_type_requirement_substitution_error : Note<
----------------
I don't think we need to talk about the mechanics of how we formed the type here. Also, adding a manual 'aka' like this will result in a double-aka (`decltype((foo))' (aka 'bar' (aka 'baz'))`) in some cases.

Aside [not something to address in this patch]: this (and other diagnostics nearby) also violate our [diagnostics best practices](https://clang.llvm.org/docs/InternalsManual.html#the-format-string) by redundantly including in the diagnostic the source expression that the caret will be pointing to, but perhaps that's justifiable in this case because we expect these expressions to be short and they're so central to what's being diagnosed.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:448
                diag::note_expr_requirement_constraints_not_satisfied_simple)
-            << (int)First << S.BuildDecltypeType(Req->getExpr(),
-                                                 Req->getExpr()->getBeginLoc())
+            << (int)First << e << S.getCanonicalTypeForParenthesizedExpr(e)
             << ConstraintExpr->getNamedConcept();
----------------
Please don't canonicalize the type here. If we've retained type sugar for the type of `e`, it'll be more helpful to the user if we present that type sugar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98160