<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 3, 2015 at 8:58 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><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 dir="ltr"><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Tue, Sep 1, 2015 at 12:31 AM Olivier Goffart <<a href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>> 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">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" target="_blank">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, the<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 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 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. normal<br>
name.<br>
<br>
For example, if you dumpt the AST of:<br>
<br>
 namespace foo { class A {} *a1 = new foo::A , *a2 = new A; }<br>
<br>
You get:<br>
<br>
[...]<br>
| |-VarDecl 0x1c9a7a0 <col:17, col:43> col:29 a1 'class A *' cinit<br>
| | `-CXXNewExpr 0x1ce2648 <col:34, col:43> 'foo::A *'<br>
| |   `-CXXConstructExpr 0x1ce2618 <col:38> 'foo::A':'class foo::A' 'void<br>
(void) throw()'<br>
| `-VarDecl 0x1ce26d0 <col:17, col:57> col:48 a2 'class A *' cinit<br>
|   `-CXXNewExpr 0x1ce2768 <col:53, col:57> 'class foo::A *'<br>
|     `-CXXConstructExpr 0x1ce2738 <col:57> 'class foo::A' 'void (void)<br>
throw()'<br>
<br>
<br>
As you can see, when the type of CXXNewExpr is fully quialified, the '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 solve.<br>
The reason I used 'using namespace foo' in the test was just so the line with<br>
A and D have the same size.   I just copy pasted the thing without thinking<br>
about that.<br></blockquote><div><br></div></div></div><div>Makes sense. In that case I think it looks good, adding Richard to cross-check for the final approval.</div></div></div>
</blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+  auto ConstructExpr = dyn_cast<CXXConstructExpr>(Op.SrcExpr.get());</div><div class="gmail_extra">+  if (auto BindExpr = dyn_cast<CXXBindTemporaryExpr>(Op.SrcExpr.get()))</div><div class="gmail_extra">+    ConstructExpr = dyn_cast_or_null<CXXConstructExpr>(BindExpr->getSubExpr());</div><div class="gmail_extra">+  if (ConstructExpr)</div><div><br></div><div>It's slightly better to write this as:</div><div><br></div><div>  auto *SubExpr = Op.SrcExpr.get();</div><div>  if (auto *BindExpr = dyn_cast<CXXBindTemporaryExpr>(SubExpr))</div><div>    SubExpr = BindExpr->getSubExpr();</div><div>  if (auto *ConstructExpr = dyn_cast<CXXConstructExpr>(SubExpr))</div></div><div class="gmail_extra"><br></div><div class="gmail_extra">Otherwise, LGTM.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">[The root cause of the problem here is suboptimal design of this AST node. It doesn't make sense for both CXXFunctionalCastExpr and CXXConstructExpr to hold the locations of the same set of parens (and we can see from StmtPrinter that CXXFunctionalCastExpr currently "owns" the parens). But fixing that is a larger task, so let's just get this immediate bug fixed for now.]</div></div>