[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:559
+  // Get the resolved template arguments from the canonical type.
+  // FIXME: Handle the as-written arguments so the sugar is not lost.
+  ArrayRef<TemplateArgument> PResolved =
----------------
rsmith wrote:
> Does this matter on the `P` side? (Can we remove this FIXME?)
Even though P sugar is not relevant to the deduced type, it is relevant to diagnostics when there is a deduction failure. This has been relevant in the other parts of this patch, so I don't see why we would not at least want to preserve it here, though it might be too much trouble to be worth the effort.

I will keep the comment for now, if we have any discussion where it becomes clear there would be no difference here, I will make a one line NFC and remove it.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:561
+  ArrayRef<TemplateArgument> PResolved =
+      TP->getCanonicalTypeInternal()
+          ->castAs<TemplateSpecializationType>()
----------------
rsmith wrote:
> Can we avoid using the `Internal` version here? (The only difference is whether we preserve top-level qualifiers that are outside any type sugar.)
> 
> Might also be worth a comment here explaining that we're going to the canonical type first here to step over any alias templates.
The Internal version is more convenient because TP is a `Type *` not a `QualType`, so the suggestion as you have written would not work.

Removing the alias template would be handled by `getUnqualifiedDesugaredType`, above, which my understanding is that will remove all top level sugar.

But I see that I went all this way around to preserving the non-canonical TP but all I needed really from it was the templateName, which probably is always the same as the canonical one, though not sure if that is by design.

I just made a change to get the canonical type on TP above instead, and we can circle back to this later.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:571-573
+  // FIXME: Should not lose sugar here.
+  if (const auto *SA = dyn_cast<TemplateSpecializationType>(
+          UA->getCanonicalTypeInternal())) {
----------------
rsmith wrote:
> I think we can retain type sugar here without too much effort.
That does not work, blows up some 80 tests.
I will leave the FIXME and circle back to this after I am done with my pile of patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216