[PATCH] D127695: [clang] Implement Template Specialization Resugaring

[David Rector via Phabricator via cfe-commits]

davrec added a subscriber: rjmccall.
davrec added a comment.

First thank you for having separated out the public AST changes into other patches, it makes these mostly-Sema changes much easier to review.

I don't see any major issues with the code, though this would benefit from a close look from more folks with deeper Sema familiarity.  Maybe @rjmccall would be willing or could @ someone?

The larger issues:

1. Performance - can we see some numbers?  and
2. There are a lot of FIXMEs introduced - understandable because of the scale, but it would be nice to hear you commit to eventually getting to those because otherwise they will probably remain indefinitely.   @aaron.ballman should probably weigh in/sign off on this one given the number of FIXMEs, which probably can't be handled before @mizvekov 's deadline (when is that again?).



================
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:879-882
+  TypeLocBuilder TLB;
+  DecltypeTypeLoc DecltypeTL = TLB.push<DecltypeTypeLoc>(T);
+  DecltypeTL.setDecltypeLoc(DS.getTypeSpecTypeLoc());
+  DecltypeTL.setRParenLoc(DS.getTypeofParensRange().getEnd());
----------------
Move back down?


================
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:993-999
+  // Alias template specializations can produce types which are not valid
+  // nested name specifiers.
+  if (!T->isDependentType() && !T->getAs<TagType>()) {
+    Diag(TemplateNameLoc, diag::err_nested_name_spec_non_tag) << T;
+    NoteAllFoundTemplates(Template);
+    return true;
+  }
----------------
Move back up?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:3427-3430
     if (unsigned BID = cast<FunctionDecl>(VD)->getBuiltinID()) {
       if (!Context.BuiltinInfo.isDirectlyAddressable(BID)) {
         type = Context.BuiltinFnTy;
         valueKind = VK_PRValue;
----------------
Does this need the same changes as Sema::FixOverloadedFunctionReference?  (see below)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:20800
         DRE->copyTemplateArgumentsInto(TemplateArgs);
+        // FIXME: resugar
         return BuildDeclRefExpr(
----------------
Could this be done same as earlier (lines 19548-19553)?


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1862
 
     Qualifiers MemberQuals =
+        Context.getCanonicalType(FieldType).getQualifiers();
----------------
Rename to FieldQuals


================
Comment at: clang/lib/Sema/SemaOverload.cpp:15357
 
     // FIXME: Duplicated from BuildDeclarationNameExpr.
+    QualType Type;
----------------
I think this should have said "Duplicated from diagnoseUncapturableValueReferenceOrBinding" - if so do we need the below changes over there too?


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:743
+      }
+      llvm_unreachable("");
+    }
----------------
Add message


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:95
 
+TemplateDecl *getTemplateDecl(NamedDecl *D) {
+  switch (D->getKind()) {
----------------
static/move to namespace {} below.  Or make it a public method of Decl?  `Decl::getOriginalDescribedTemplate()` or something like that?  (If you go that route then best to fall back to getDescribedTemplate instead of the unreachable.)


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:130
+    // Maybe fall back to Decl::getDescribedTemplate.
+    D->dumpColor();
+    llvm_unreachable("Unhandled decl kind");
----------------
Not sure of policy, should this dump should be removed?  Several others below too.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:161
+public:
+  struct SemanticContextRAII {
+    SemanticContextRAII(Resugarer &R, NamedDecl *ND,
----------------
Wouldn't hurt to add a brief comment above each RAII class describing its role


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:257
+      assert(!Args.empty());
+      if (Reverse)
+        R->CurTemplateToArgs.try_emplace(Template, Args);
----------------
Would be nice to have a comment explaining the effect of Reverse here or above addTypeToMap.  (Reverse iteration?)


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:265
+      struct {
+        NamedDecl *ND;
+        const TemplateSpecializationType *TS;
----------------
Could clarify to CXXRecordDecl


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:293
+          T->dump();
+          llvm_unreachable("");
+        }
----------------
Add message


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:466
+      Result = SemaRef.Context.getElaboratedType(
+          T->getKeyword(), QualifierLoc.getNestedNameSpecifier(), NamedT);
+
----------------
Maybe include T->OwnedTagDecl arg for consistency with deduction resugaring


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:479
+    const SubstTemplateTypeParmType *T = TL.getTypePtr();
+    Decl *ReplacedDecl = T->getAssociatedDecl();
+    Optional<unsigned> PackIndex = T->getPackIndex();
----------------
AssociatedDecl


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4783
     int64_t N = Index.getExtValue();
-    return getSubstType(Ts.getPackAsArray()[N].getAsType(), 1,
-                        Ts.pack_size() - 1 - N);
+    // FIXME: resugaring here is actually unnecessary.
+    return getResugaredTemplateSpecializationType(
----------------
Change back to original?


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:5371-5386
+  // Check the tag kind
+  if (const RecordType *RT = T->getAs<RecordType>()) {
+    RecordDecl *D = RT->getDecl();
+
+    IdentifierInfo *Id = D->getIdentifier();
+    assert(Id && "templated class must have an identifier");
+
----------------
Move back up?


================
Comment at: clang/lib/Sema/TreeTransform.h:2679-2681
+    CXXScopeSpec SS;
+    SS.Adopt(QualifierLoc);
+
----------------
Move back down?


================
Comment at: clang/test/Sema/Resugar/resugar-expr.cpp:244
+// N-error at -2 {{with an rvalue of type 'int'}}
+} // namespace t21
----------------
mizvekov wrote:
> davrec wrote:
> > Compositions of MemberExprs/CXXMemberCallExprs have an issue:
> > ```
> > template <class A1> struct A {
> >   struct Inner {
> >     A1 m;
> >     A1 f();
> >   } inner;
> >   Inner g();
> > };
> > Z x1 = A<Int>().inner.m; //No resugar
> > Z x2 = A<Int>().inner.f(); //No resugar
> > Z x3 = A<Int>().g().m; //No resugar
> > Z x4 = A<Int>().g().f(); //No resugar
> > Z x5 = A<Int>::Inner().m; //ok
> > ```
> > 
> > Composed `CallExprs` seem to work but probably warrant a test, e.g.
> > ```
> > template <class B1> B1 h(B1);
> > Z x6 = h(Int());
> > Z x7 = h(h(Int()));
> > ```
> > 
> > https://godbolt.org/z/cszrsvh8d
> > 
> Thanks for the feedback!
> 
> The problem here I think is that for a MemberExpr, we have to look at not only the BaseType of it for a resugaring context, but instead look at its BaseExpr to see if it or any bases of it, and so on recursively, can provide a resugaring context as well, effectively building one naming context from the whole composition.
> 
> I will try to address that later, I probably should work on facilitating the reviews of the other patches in this stack, and then splitting this one big patch up more :)
Inheritance is also an issue:

https://godbolt.org/z/81nPqzsrd

```
using Int = int;
enum class Z;

template<typename A1>
struct A {
    A1 m;
};

template<typename A1>
struct B : A<A1> {};

Z x2 = B<Int>().m; //doesn't resugar
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127695