[PATCH] Fix missing source location in CXXTemporaryObjectExpr.

[Eli Friedman]

On Thu, Sep 5, 2013 at 12:34 AM, Enea Zaffanella <zaffanella at cs.unipr.it>wrote:

> On 09/04/2013 11:05 PM, Eli Friedman wrote:
>
>> On Wed, Sep 4, 2013 at 4:24 AM, Enea Zaffanella <zaffanella at cs.unipr.it
>> <mailto:zaffanella at cs.unipr.it**>> wrote:
>>
>>     When dumping the following C++11 chunk of code
>>
>>     $ cat chunk.cc
>>     struct A { A(int, int); };
>>     A a_paren( A(0,0) );
>>     A a_range( A{0,0} );
>>
>>     the CXXTemporaryObjectExpr generated by A{0,0}
>>     is missing a proper end location
>>     (the source location is correctly set for A(0,0)).
>>
>>     $ clang -cc1 -std=c++11 -ast-dump chunk.cc
>>     [...]
>>     `-VarDecl 0x5a963e0 <line:3:1, col:19> a_range 'struct A'
>>        `-CXXConstructExpr 0x5a96570 <col:3, col:19> 'struct A' 'void
>>     (struct A &&) noexcept' elidable
>>          `-MaterializeTemporaryExpr 0x5a96550 <col:12, <invalid sloc>>
>>     'struct A' xvalue
>>            `-CXXTemporaryObjectExpr 0x5a964d8 <col:12, <invalid sloc>>
>>     'struct A' 'void (int, int)'
>>              |-IntegerLiteral 0x5a96448 <col:14> 'int' 0
>>              `-IntegerLiteral 0x5a96468 <col:16> 'int' 0
>>
>>
>>     Please find attached a patch (with testcase) fixing this issue.
>>     OK to commit?
>>
>>
>> +    if (Kind.getKind() == InitializationKind::IK_**DirectList)
>> +      ParenRange = SourceRange(LBraceLoc, RBraceLoc);
>> +    else
>>         ParenRange = Kind.getParenRange();
>>
>> This isn't right: there aren't any parentheses.  getParenRange() on a
>> CXXTemporaryObjectExpr shouldn't return source locations pointing at
>> tokens which aren't parentheses.
>>
>> You should be able to fix this purely in
>> CXXTemporaryObjectExpr::**getLocStart()/getLocEnd().
>>
>> -Eli
>>
>
>
> I see your point but ...
>
> My guess is that getParenRange() method was designed considering only
> C++03; now that we also have C++11 list initialization, I was freely
> reinterpreting it as meaning "getParenOrBraceRange()".
>
> Note that, in the dump above, the arguments of the CXXTemporaryObjectExpr
> node are the elements of the InitListExpr.
> That is, the InitListExpr node is unwrapped before creating the temporary
> object node. So, if the getParenRange() is interpreted to strictly mean
> only parentheses (not braces), we end up having no place to store the
> source location for the braces themselves (and hence no place to store the
> end location of the node). That is, we can not query the arguments to
> obtain the end location (which is the trick used in
> CXXConstructExpr::getLocEnd).
>
> To summarize, I see two options:
>
> (a) rename getParenRange() as getParenOrBraceRange();
>
> (b) have two different methods, getParenRange() and getBraceRange(), to be
> used based on the value of a boolean flag.
> Question: is it the case that
>   bool CXXConstructExpr::**isListInitialization()
> is such an appropriate flag to be used here?
>

Yes.


> I would probably opt for (b).
> Do you see a third option which is better?
>

I don't really see any other options; I guess I would lean towards option
A, and let the users branch if they care about the difference, but either
way works.


>
> Enea.
>
>
> P.S.: while playing on variations of the testcase in the patch, I noticed
> the following:
>
> # cat bug.cc
> struct A {
>   A(const int(&)[1]);
> };
>
> A a_paren( A( {0} ) );
>
> A a_range( A{ {0} } );
>
> When dumping `a_paren', we see that A( {0} ) produces a
> CXXFunctionaCastExpr node (which is fine).
>
> In contrast, when dumping `a_range' we see that A{ {0} } produces a
> CXXTemporaryObjectExpr node. This does not match the doxygen documentation
> for this AST node, where it is said:
>
> > Represents a C++ functional cast expression that builds a
> > temporary object.
> >
> > This expression type represents a C++ "functional" cast
> > (C++[expr.type.conv]) with N != 1 arguments that invokes a
> > constructor to build a temporary object. With N == 1 arguments the
> > functional cast expression will be represented by
> > CXXFunctionalCastExpr.
>
> Is the doxygen documentation obsolete? Or is it the case that a
> CXXFunctionalCastExpr node should have been produced for `a_range' too?
>

The doxygen just hasn't been updated for C++11.  CXXFunctionalCastExpr
represents an explicit type conversion which is equivalent to a cast per
[expr.type.conv]p1, and nothing else.

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130905/a0c764dd/attachment.html>