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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added inline comments.


================
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<
----------------
rsmith wrote:
> 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.
Though the original code just built a decltype and printed it here. This format string, as far as I remember, matches exactly what the original code printed, so that a sweeping change to update all the tests would not have to be made.
I vaguely remember following the code of the type printer here and seeing that it would stop from printing an aka chain by just canonicalizing the type on the first aka.

So yeah, the original code ironically violated this best practice by just printing a type, but which in this case contains the expression.

But I agree we do not necessarily need to explain the mechanics of this type here, though I think the standard does explain how the type is obtained here in terms of `decltype(())`.

But since you mentioned this, I think the best approach here might be to just print the non canonical type, and not print the expression.
Will have to update all the tests but no big deal. Agreed?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98160