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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked 12 inline comments as done.
mizvekov added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:180
           )cpp",
-          // FIXME: it'd be nice if this resolved to the alias instead
-          "struct Foo",
+          // It's so nice that this is resolved to the alias instead :-D
+          "Bar",
----------------
rsmith wrote:
> Very true. But probably not worth keeping the comment. :)
Such a party pooper =)


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350-1362
+  QualType ParDesug;
+  auto updatePar = [&Param, &ParDesug, &S](QualType T) {
+    Param = T;
+    ParDesug = T.getDesugaredType(S.Context);
+  };
+  updatePar(Param);
+
----------------
rsmith wrote:
> rsmith wrote:
> > `Par` is an unusual name for a parameter type; `Parm` or `Param` would be more common and I'd find either of those to be clearer. (Ideally use the same spelling for `Param` and `ParDesug`.) Given that the description in the standard uses `P` and `A` as the names of these types, those names would be fine here too if you want something short.
> Instead of tracking two types here, I think it'd be clearer to follow the more usual pattern in Clang: track only `Param` and `Arg`, use `Arg->getAs<T>()` instead of `dyn_cast<T>(ArgDesug)` to identify if `Arg` is sugar for a `T`, and use `Arg->getAs<T>()` instead of `cast<T>(ArgDesug)` to get a minimally-desugared `T*` from `Arg`.
> 
> The only place where I think we'd want something different from that is in the `switch (Param->getTypeClass())` below, where I think we should switch on the type class of the canonical type (or equivalently on the type class of the desugared type, but it's cheaper and more obviously correct to ask for the type class of the canonical type).
There was one small catch in your suggestion:
Using getAs on TemplateSpecializationType is a bit tricky because that type can be sugar as well, so you might end up with a type alias instead of the underlying thing you wanted. I think that was the only tricky type though.
And it was productive that I ran into this problem, because I ended up discovering some cases where we were losing some sugar there, and there was a tiny diagnostic improvement in one of the test cases as a result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216