<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 3, 2015 at 2:26 PM, Olivier Goffart <span dir="ltr"><<a href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thursday 3. September 2015 13:41:12 Richard Smith wrote:<br>
> On Thu, Sep 3, 2015 at 8:58 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> > On Tue, Sep 1, 2015 at 12:31 AM Olivier Goffart <<a href="mailto:ogoffart@kde.org">ogoffart@kde.org</a>> wrote:<br>
> >> On Monday 31. August 2015 08:07:58 Manuel Klimek wrote:<br>
> >> > On Sat, Aug 29, 2015 at 12:23 PM Olivier Goffart via cfe-commits <<br>
> >> ><br>
> >> > <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
> >> > > Hi,<br>
> >> > ><br>
> >> > > Please review the attached patch.<br>
> >> > ><br>
> >> > > In Sema::BuildCXXFunctionalCastExpr, if the class has a destructor,<br>
> >><br>
> >> the<br>
> >><br>
> >> > > Op.SrcExpr might be a CXXBindTemporaryExpr which we need to unwrap.<br>
> >> > ><br>
> >> > > In the testcase, the first new CHECK worked (because A does not have<br>
> >> > > a<br>
> >> > > destructor),  but the second CHECK failed (did not include the last<br>
> >> > > parenthese) because D has a destructor.<br>
> >> > ><br>
> >> > > I used dyn_cast_or_null just to be safe, becasue i don't know if it<br>
> >> > > is<br>
> >> > > possible for the BindExpr->getSubExpr() to be null.<br>
> >> ><br>
> >> > Do you know why the added test for A says 'class foo::A' instead of<br>
> >> > 'foo::A' as the other tests do in that file?<br>
> >><br>
> >> I don't know.  It seems it has to do with the fully quallified name vs.<br>
> >> normal<br>
> >> name.<br>
> >><br>
> >> For example, if you dumpt the AST of:<br>
> >>  namespace foo { class A {} *a1 = new foo::A , *a2 = new A; }<br>
> >><br>
> >> You get:<br>
> >><br>
> >> [...]<br>
> >><br>
> >> | |-VarDecl 0x1c9a7a0 <col:17, col:43> col:29 a1 'class A *' cinit<br>
> >> | |<br>
> >> | | `-CXXNewExpr 0x1ce2648 <col:34, col:43> 'foo::A *'<br>
> >> | |<br>
> >> | |   `-CXXConstructExpr 0x1ce2618 <col:38> 'foo::A':'class foo::A' 'void<br>
> >><br>
> >> (void) throw()'<br>
> >><br>
> >> | `-VarDecl 0x1ce26d0 <col:17, col:57> col:48 a2 'class A *' cinit<br>
> >> |<br>
> >> |   `-CXXNewExpr 0x1ce2768 <col:53, col:57> 'class foo::A *'<br>
> >> |<br>
> >> |     `-CXXConstructExpr 0x1ce2738 <col:57> 'class foo::A' 'void (void)<br>
> >><br>
> >> throw()'<br>
> >><br>
> >><br>
> >> As you can see, when the type of CXXNewExpr is fully quialified, the<br>
> >> 'class'<br>
> >> keyword is omited, but for a2, it prints the 'class' keyword.<br>
> >> The printed type of CXXConstructExpr is even more wierd when it is fully<br>
> >> qualified.<br>
> >><br>
> >><br>
> >> I guess that's because of ElaboratedTypePolicyRAII in TypePrinter.cpp<br>
> >><br>
> >> But this is irrelevant for this patch and the problem it's trying to<br>
> >> solve.<br>
> >> The reason I used 'using namespace foo' in the test was just so the line<br>
> >> with<br>
> >> A and D have the same size.   I just copy pasted the thing without<br>
> >> thinking<br>
> >> about that.<br>
> ><br>
> > Makes sense. In that case I think it looks good, adding Richard to<br>
> > cross-check for the final approval.<br>
><br>
> +  auto ConstructExpr = dyn_cast<CXXConstructExpr>(Op.SrcExpr.get());<br>
> +  if (auto BindExpr = dyn_cast<CXXBindTemporaryExpr>(Op.SrcExpr.get()))<br>
> +    ConstructExpr =<br>
> dyn_cast_or_null<CXXConstructExpr>(BindExpr->getSubExpr());<br>
> +  if (ConstructExpr)<br>
><br>
> It's slightly better to write this as:<br>
><br>
>   auto *SubExpr = Op.SrcExpr.get();<br>
>   if (auto *BindExpr = dyn_cast<CXXBindTemporaryExpr>(SubExpr))<br>
>     SubExpr = BindExpr->getSubExpr();<br>
>   if (auto *ConstructExpr = dyn_cast<CXXConstructExpr>(SubExpr))<br>
><br>
> Otherwise, LGTM.<br>
<br>
<br>
</div></div>Thanks.<br>
<br>
So no dyn_cast_or_null?   CXXBindTemporaryExpr::getSubExpr never returns null<br>
here?<br>
<span class=""><br>
> [The root cause of the problem here is suboptimal design of this AST node.<br>
> It doesn't make sense for both CXXFunctionalCastExpr and CXXConstructExpr<br>
> to hold the locations of the same set of parens (and we can see from<br>
> StmtPrinter that CXXFunctionalCastExpr currently "owns" the parens). But<br>
> fixing that is a larger task, so let's just get this immediate bug fixed<br>
> for now.]<br>
<br>
</span>[This is however quite convnient for me to get the location of the parentheses<br>
of a CXXConstructorExpr. There are may cases where CXXConstructorExpr is not<br>
in a CXXFunctionalCastExpr]</blockquote><div><br></div><div>There are also many cases where it doesn't involve parens =)</div><div><br></div><div>The problem is that for a declaration of the form:</div><div><br></div><div>  T x(y);</div><div><br></div><div>... the parens are owned by the VarDecl, because we don't have a separate AST representation for "direct initialization of scalar type" to hold the parens. Consistency with that implies CXXConstructExprs do not own their own parens, and it doesn't make sense to store those paren locations on the CXXConstructExpr node as a result.</div><div><br></div><div>Conversely, for a declaration of the form:</div><div><br></div><div>  T x{y};</div><div><br></div><div>... the braces are owned by the initialization expression, not by the VarDecl, either as an InitListExpr or by the CXXConstructExpr representing the list initialization.</div><div><br></div><div>The most natural approach to resolve this tension would be to add a new AST node for non-class direct initialization, and to add subclasses of CXXConstructExpr for different syntaxes (and remove the redundant and conflicting CXXTemporaryObjectExpr node). But this is a fairly large restructuring, and I've not found time to do it yet.</div><div><br></div><div>(If you're interested in making a start on this, removing CXXTemporaryObjectExpr seems like a good first step. In its place, create a CXXFunctionalCastExpr wrapping a CXXConstructExpr.)</div></div></div></div>