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

[Michael Han]

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 <fragmentshaders at gmail.com>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 <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/20130618/3aa3d6f2/attachment.html>