[PATCH] D133874: [clang] Changes to produce sugared converted template arguments

[Erich Keane via Phabricator via cfe-commits]

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