[PATCH] Fix missing source location in CXXTemporaryObjectExpr.

[Enea Zaffanella]

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?

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

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?