On Thu, Sep 5, 2013 at 12:34 AM, Enea Zaffanella <[email protected]>wrote:
> On 09/04/2013 11:05 PM, Eli Friedman wrote: > >> On Wed, Sep 4, 2013 at 4:24 AM, Enea Zaffanella <[email protected] >> <mailto:[email protected]**>> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
