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@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits