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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
