<div dir="ltr">On Thu, Sep 5, 2013 at 12:34 AM, Enea Zaffanella <span dir="ltr"><<a href="mailto:zaffanella@cs.unipr.it" target="_blank">zaffanella@cs.unipr.it</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On 09/04/2013 11:05 PM, Eli Friedman wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
On Wed, Sep 4, 2013 at 4:24 AM, Enea Zaffanella <<a href="mailto:zaffanella@cs.unipr.it" target="_blank">zaffanella@cs.unipr.it</a><br></div><div><div class="h5">
<mailto:<a href="mailto:zaffanella@cs.unipr.it" target="_blank">zaffanella@cs.unipr.it</a><u></u>>> wrote:<br>
<br>
When dumping the following C++11 chunk of code<br>
<br>
$ cat chunk.cc<br>
struct A { A(int, int); };<br>
A a_paren( A(0,0) );<br>
A a_range( A{0,0} );<br>
<br>
the CXXTemporaryObjectExpr generated by A{0,0}<br>
is missing a proper end location<br>
(the source location is correctly set for A(0,0)).<br>
<br>
$ clang -cc1 -std=c++11 -ast-dump chunk.cc<br>
[...]<br>
`-VarDecl 0x5a963e0 <line:3:1, col:19> a_range 'struct A'<br>
`-CXXConstructExpr 0x5a96570 <col:3, col:19> 'struct A' 'void<br>
(struct A &&) noexcept' elidable<br>
`-MaterializeTemporaryExpr 0x5a96550 <col:12, <invalid sloc>><br>
'struct A' xvalue<br>
`-CXXTemporaryObjectExpr 0x5a964d8 <col:12, <invalid sloc>><br>
'struct A' 'void (int, int)'<br>
|-IntegerLiteral 0x5a96448 <col:14> 'int' 0<br>
`-IntegerLiteral 0x5a96468 <col:16> 'int' 0<br>
<br>
<br>
Please find attached a patch (with testcase) fixing this issue.<br>
OK to commit?<br>
<br>
<br>
+ if (Kind.getKind() == InitializationKind::IK_<u></u>DirectList)<br>
+ ParenRange = SourceRange(LBraceLoc, RBraceLoc);<br>
+ else<br>
ParenRange = Kind.getParenRange();<br>
<br>
This isn't right: there aren't any parentheses. getParenRange() on a<br>
CXXTemporaryObjectExpr shouldn't return source locations pointing at<br>
tokens which aren't parentheses.<br>
<br>
You should be able to fix this purely in<br>
CXXTemporaryObjectExpr::<u></u>getLocStart()/getLocEnd().<br>
<br>
-Eli<br>
</div></div></blockquote>
<br>
<br>
I see your point but ...<br>
<br>
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()".<br>
<br>
Note that, in the dump above, the arguments of the CXXTemporaryObjectExpr node are the elements of the InitListExpr.<br>
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).<br>
<br>
To summarize, I see two options:<br>
<br>
(a) rename getParenRange() as getParenOrBraceRange();<br>
<br>
(b) have two different methods, getParenRange() and getBraceRange(), to be used based on the value of a boolean flag.<br>
Question: is it the case that<br>
bool CXXConstructExpr::<u></u>isListInitialization()<br>
is such an appropriate flag to be used here?<br></blockquote><div><br></div><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I would probably opt for (b).<br>
Do you see a third option which is better?<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Enea.<br>
<br>
<br>
P.S.: while playing on variations of the testcase in the patch, I noticed the following:<br>
<br>
# cat bug.cc<br>
struct A {<br>
A(const int(&)[1]);<br>
};<br>
<br>
A a_paren( A( {0} ) );<br>
<br>
A a_range( A{ {0} } );<br>
<br>
When dumping `a_paren', we see that A( {0} ) produces a CXXFunctionaCastExpr node (which is fine).<br>
<br>
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:<br>
<br>
> Represents a C++ functional cast expression that builds a<br>
> temporary object.<br>
><br>
> This expression type represents a C++ "functional" cast<br>
> (C++[expr.type.conv]) with N != 1 arguments that invokes a<br>
> constructor to build a temporary object. With N == 1 arguments the<br>
> functional cast expression will be represented by<br>
> CXXFunctionalCastExpr.<br>
<br>
Is the doxygen documentation obsolete? Or is it the case that a CXXFunctionalCastExpr node should have been produced for `a_range' too?<br>
</blockquote></div><br></div>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.<br><div>
<div><br><div class="gmail_extra">-Eli</div></div></div></div>