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

Matheus Izvekov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 11 14:54:27 PST 2021

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list