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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to