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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

The test changes are a little bizarre, are there any better tests you can write that shows this behavior?

Also, at the 'end' of this patch set, we should make sure we have a detailed release note.



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


================
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() {
----------------
Not your fault at all.... but that seems to be about the absolute opposite of what it looks like this function is doing...


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133874