Ping. Hi Richard, do you have a chance to take a look at this patch? Does this patch make sense or there is a better way of creating type nodes for implicit functions?
On Wed, Jun 12, 2013 at 10:40 AM, Michael Han <[email protected]>wrote: > Hi Manuel, > > Thank you for the comments! The patch is updated. > > For the test, I am only testing visiting the parameter of implicit copy > assignment function. The same test visitor can be reused to test visiting > parameter of other implicit functions like copy / move constructors. I am > only testing copy assignment operator in this patch because other implicit > functions have the same problem that needs to be fixed, and if my fix in > this patch is the right approach I will apply the fix to other implicit > functions as well, and add then add more tests. > > > > On Tue, Jun 11, 2013 at 11:37 PM, Manuel Klimek <[email protected]> wrote: > >> On Tue, Jun 11, 2013 at 10:31 PM, Michael Han >> <[email protected]>wrote: >> >>> Hi klimek, rsmith, >>> >>> This patch fixes PR16182 >>> http://llvm.org/bugs/show_bug.cgi?id=16182 >>> >>> A valid TypeSourceInfo of a function is required for the RAV to visit >>> the parameter declaration of the function, and previously we don't have a >>> valid TypeSourceInfo when creating the function declaration of implicit >>> copy assignment operator. >>> >>> This patch fixes this by building a TypeSourceInfo from the function >>> prototype of copy assignment operator and associate its ParmVarDecl with >>> this function type location. >>> >>> The test is done by matching that an empty name (we don't have a name >>> for the parameter of the implicit copy assignment function) appears at >>> expect location, which is the source location of the class that is also >>> used as the location of the implicit copy assignment function. >>> >>> >>> http://llvm-reviews.chandlerc.com/D958 >>> >>> Files: >>> unittests/Tooling/TestVisitor.h >>> unittests/Tooling/RecursiveASTVisitorTest.cpp >>> lib/Sema/SemaDeclCXX.cpp >>> >>> Index: unittests/Tooling/TestVisitor.h >>> =================================================================== >>> --- unittests/Tooling/TestVisitor.h >>> +++ unittests/Tooling/TestVisitor.h >>> @@ -31,7 +31,7 @@ >>> /// This is a drop-in replacement for RecursiveASTVisitor itself, with >>> the >>> /// additional capability of running it over a snippet of code. >>> /// >>> -/// Visits template instantiations (but not implicit code) by default. >>> +/// Visits template instantiations and implicit code by default. >>> template <typename T> >>> class TestVisitor : public RecursiveASTVisitor<T> { >>> public: >>> @@ -55,6 +55,10 @@ >>> return true; >>> } >>> >>> + bool shouldVisitImplicitCode() const { >>> + return true; >>> + } >>> + >>> protected: >>> virtual ASTFrontendAction* CreateTestAction() { >>> return new TestAction(this); >>> Index: unittests/Tooling/RecursiveASTVisitorTest.cpp >>> =================================================================== >>> --- unittests/Tooling/RecursiveASTVisitorTest.cpp >>> +++ unittests/Tooling/RecursiveASTVisitorTest.cpp >>> @@ -35,6 +35,14 @@ >>> } >>> }; >>> >>> +class ParmVarDeclVisitor : public >>> ExpectedLocationVisitor<ParmVarDeclVisitor> { >>> +public: >>> + bool VisitParmVarDecl(ParmVarDecl *ParamVar) { >>> + Match(ParamVar->getNameAsString(), ParamVar->getLocStart()); >>> + return true; >>> + } >>> +}; >>> + >>> class CXXMemberCallVisitor >>> : public ExpectedLocationVisitor<CXXMemberCallVisitor> { >>> public: >>> @@ -106,6 +114,14 @@ >>> } >>> }; >>> >>> +TEST(RecursiveASTVisitor, VisitsParmVarDecl) { >>> + ParmVarDeclVisitor Visitor; >> >> + Visitor.ExpectMatch("", 1, 7); >>> >> >> First, I'd call this test VisitsParmVarDeclForImplicitCode. >> Seconds, you're testing that it visits any parameter, not the one for >> operator==, right? >> >> I'd also add a short comment explaining this test - if I didn't know the >> full context, I'd have a hard time figuring out what it does. Especially >> explain why the name is empty, and why the position is expected to point to >> the class definition. >> >> I'll let Richard speak for the non-test portion of this patch :) >> >> >>> + EXPECT_TRUE(Visitor.runOver( >>> + "class X {};\n" >>> + "void foo(X a, X b) {a = b;}")); >>> +} >>> + >>> TEST(RecursiveASTVisitor, VisitsBaseClassDeclarations) { >>> TypeLocVisitor Visitor; >>> Visitor.ExpectMatch("class X", 1, 30); >>> Index: lib/Sema/SemaDeclCXX.cpp >>> =================================================================== >>> --- lib/Sema/SemaDeclCXX.cpp >>> +++ lib/Sema/SemaDeclCXX.cpp >>> @@ -8788,14 +8788,24 @@ >>> FunctionProtoType::ExtProtoInfo EPI; >>> EPI.ExceptionSpecType = EST_Unevaluated; >>> EPI.ExceptionSpecDecl = CopyAssignment; >>> - CopyAssignment->setType(Context.getFunctionType(RetType, ArgType, >>> EPI)); >>> + QualType T = Context.getFunctionType(RetType, ArgType, EPI); >>> + CopyAssignment->setType(T); >>> >>> + // PR16182. Build type source info for copy assignment operator. RAV >>> relies on >>> + // type source info to traverse parameter declaration of implicit >>> + // declared copy assignment operator. >>> + TypeSourceInfo *TSInfo = Context.getTrivialTypeSourceInfo(T, >>> ClassLoc); >>> + CopyAssignment->setTypeSourceInfo(TSInfo); >>> + UnqualTypeLoc UnQualTL = TSInfo->getTypeLoc().getUnqualifiedLoc(); >>> + FunctionTypeLoc FTL = UnQualTL.getAs<FunctionTypeLoc>(); >>> + >>> // Add the parameter to the operator. >>> ParmVarDecl *FromParam = ParmVarDecl::Create(Context, CopyAssignment, >>> ClassLoc, ClassLoc, >>> /*Id=*/0, >>> ArgType, /*TInfo=*/0, >>> SC_None, 0); >>> CopyAssignment->setParams(FromParam); >>> + FTL.setArg(0, FromParam); >>> >>> AddOverriddenMethods(ClassDecl, CopyAssignment); >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
