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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:8285
+                                   TemplateArgument &SugaredConverted,
+                                   TemplateArgument &CanonicalConverted,
+                                   CheckTemplateArgumentKind CTAK);
----------------
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.


================
Comment at: clang/include/clang/Sema/TemplateDeduction.h:94
 
-  /// Take ownership of the deduced template argument list.
-  TemplateArgumentList *take() {
-    TemplateArgumentList *Result = Deduced;
-    Deduced = nullptr;
+  /// Take ownership of the deduced template argument lists.
+  TemplateArgumentList *takeSugared() {
----------------
erichkeane wrote:
> Not your fault at all.... but that seems to be about the absolute opposite of what it looks like this function is doing...
Hah, it's written in the perspective of the user, not the provider.

The opposite of standard practice I guess.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4876
       Context, NamedConcept->getDeclContext(), NamedConcept->getLocation(),
-      Converted);
+      CanonicalConverted);
   ConstraintSatisfaction Satisfaction;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133874