[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.



>> OK, so it sounds like the printing behavior change (not necessarily with a policy flag) necessary for diagnostics (make it non-ambiguous/not invalid code which is what is currently emitted) would be a good bug fix both for diagnostics and for ast-print? I'm not sure we need anything other than that.
>
> Agreed, fixing up diagnostics and ast print to not print invalid types would be a good fix to have. It doesn't necessitate a new printing policy.
>
>> So that what is currently printing as `t1<{}>` (ambiguous if the NTTP is of type `auto`, non-ambiguous, but invalid C++ if the NTTP has the specific type already) prints out as `t1<t2{}>` (correct C++ and disambiguates in the case of `auto` ambiguity).
>>
>> Or would you want the non-auto case to print in diagnostics as `t1<{}>` since there's no ambiguity, despite that not being valid C++? And then have a separate policy for ast-print that still prints it as `t1<t2{}>` so it's valid code?
>
> Personally (and others are welcome to disagree with me here!), I think we would want to print `t1<t2{}>` even in the `auto` case for diagnostics and ast print.

(sorry to nitpick, but this conversation's been a bit confusing so trying to be extra clear) - I guess you meant "even in the *non*-`auto`" case? (the auto case I think is clearer that it should always have the type - otherwise it's ambiguous) - but yeah, I'm with you - I think the language requires the type even in the non-auto case and it seems reasonable to side with that for explicitness/clarity even if it could arguably be omitted without loss of information, technically.

> For diagnostics, we sometimes print type information reasonably far away from where the declaration of a type is (and use a note to shift the user's gaze to the related type), and so I think having the extra type information is useful for that situation. But even when we print the type information reasonably close to the declaration of the type, it can be helpful because the code context might be easy to lose -- consider using Clang from an IDE where diagnostics are added to a listbox; if you copy the diagnostic from the listbox to paste into an email to a coworker to ask about it, the related code isn't going to come along without extra intervention.

Ok, so sounds like we're on the same page that `t1<{}>` is a diagnostic quality bug and we'd love to see a fix for it to include the top level type of the NTTP, as in `t1<t2{}>`, and that shouldn't need a new printing policy - because we never want to print `t1<{}>` and anywhere we do is a bug to be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453