erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/include/clang/Sema/Sema.h:8285
+ TemplateArgument &SugaredConverted,
+ TemplateArgument &CanonicalConverted,
+ CheckTemplateArgumentKind CTAK);
----------------
mizvekov wrote:
> erichkeane wrote:
> > I find myself wondering if we should just be passing around the 'Sugared'
> > version, and canonicalizing when we need it? Is there a good reason not to?
> I think given the size of the patch, splitting things this way makes it way
> easier to not pass the wrong list somewhere, and then spend a long time
> looking for the problem.
>
> I plan to make this better in a future patch, I am investigating the net
> benefits of tracking and uniquing template argument lists, much like how we
> do for types.
>
> Then, I think we can get rid of this duplication in all places that are just
> passing the lists around.
>
> Otherwise, llvm-compile-time-tracker shows no performance impact from this
> patch at all (and the same for all the others in the sugared substitution
> series), so I did not want to block everything on that extra work.
SGTM.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4876
Context, NamedConcept->getDeclContext(), NamedConcept->getLocation(),
- Converted);
+ CanonicalConverted);
ConstraintSatisfaction Satisfaction;
----------------
mizvekov wrote:
> erichkeane wrote:
> > At one point I wonder if there is value to storing the sugared version here
> > instead. We currently just create these every time we need them, so the
> > side-effect would be possibly a nicer looking AST dump. BUT we don't
> > really canonizalize these Specialization Decls/Specialization Exprs.
> That will happen in D136566, which you already reviewed.
>
> If we did that before the changes in D134604, then this would lead to a crash
> for certain test cases involving partial substitutions.
Ah! Hoisted by my own poor memory again! Thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133874/new/
https://reviews.llvm.org/D133874
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
