[PATCH] PR16182 - Visit parameter declaration of implicitly declared copy assignment operator.

[Michael Han]

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 <klimek at google.com> wrote:

> On Tue, Jun 11, 2013 at 10:31 PM, Michael Han <fragmentshaders at gmail.com>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);
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130612/aa926fe4/attachment.html>