[PATCH] fix parentheses location in a CXXConstructExpr

[Richard Smith via cfe-commits]

On Thu, Sep 3, 2015 at 8:58 AM, Manuel Klimek <klimek at google.com> wrote:

> On Tue, Sep 1, 2015 at 12:31 AM Olivier Goffart <ogoffart at kde.org> wrote:
>
>> On Monday 31. August 2015 08:07:58 Manuel Klimek wrote:
>> > On Sat, Aug 29, 2015 at 12:23 PM Olivier Goffart via cfe-commits <
>> >
>> > cfe-commits at lists.llvm.org> wrote:
>> > > Hi,
>> > >
>> > > Please review the attached patch.
>> > >
>> > > In Sema::BuildCXXFunctionalCastExpr, if the class has a destructor,
>> the
>> > > Op.SrcExpr might be a CXXBindTemporaryExpr which we need to unwrap.
>> > >
>> > > In the testcase, the first new CHECK worked (because A does not have a
>> > > destructor),  but the second CHECK failed (did not include the last
>> > > parenthese) because D has a destructor.
>> > >
>> > > I used dyn_cast_or_null just to be safe, becasue i don't know if it is
>> > > possible for the BindExpr->getSubExpr() to be null.
>> >
>> > Do you know why the added test for A says 'class foo::A' instead of
>> > 'foo::A' as the other tests do in that file?
>>
>> I don't know.  It seems it has to do with the fully quallified name vs.
>> normal
>> name.
>>
>> For example, if you dumpt the AST of:
>>
>>  namespace foo { class A {} *a1 = new foo::A , *a2 = new A; }
>>
>> You get:
>>
>> [...]
>> | |-VarDecl 0x1c9a7a0 <col:17, col:43> col:29 a1 'class A *' cinit
>> | | `-CXXNewExpr 0x1ce2648 <col:34, col:43> 'foo::A *'
>> | |   `-CXXConstructExpr 0x1ce2618 <col:38> 'foo::A':'class foo::A' 'void
>> (void) throw()'
>> | `-VarDecl 0x1ce26d0 <col:17, col:57> col:48 a2 'class A *' cinit
>> |   `-CXXNewExpr 0x1ce2768 <col:53, col:57> 'class foo::A *'
>> |     `-CXXConstructExpr 0x1ce2738 <col:57> 'class foo::A' 'void (void)
>> throw()'
>>
>>
>> As you can see, when the type of CXXNewExpr is fully quialified, the
>> 'class'
>> keyword is omited, but for a2, it prints the 'class' keyword.
>> The printed type of CXXConstructExpr is even more wierd when it is fully
>> qualified.
>>
>>
>> I guess that's because of ElaboratedTypePolicyRAII in TypePrinter.cpp
>>
>> But this is irrelevant for this patch and the problem it's trying to
>> solve.
>> The reason I used 'using namespace foo' in the test was just so the line
>> with
>> A and D have the same size.   I just copy pasted the thing without
>> thinking
>> about that.
>>
>
> Makes sense. In that case I think it looks good, adding Richard to
> cross-check for the final approval.
>

+  auto ConstructExpr = dyn_cast<CXXConstructExpr>(Op.SrcExpr.get());
+  if (auto BindExpr = dyn_cast<CXXBindTemporaryExpr>(Op.SrcExpr.get()))
+    ConstructExpr =
dyn_cast_or_null<CXXConstructExpr>(BindExpr->getSubExpr());
+  if (ConstructExpr)

It's slightly better to write this as:

  auto *SubExpr = Op.SrcExpr.get();
  if (auto *BindExpr = dyn_cast<CXXBindTemporaryExpr>(SubExpr))
    SubExpr = BindExpr->getSubExpr();
  if (auto *ConstructExpr = dyn_cast<CXXConstructExpr>(SubExpr))

Otherwise, LGTM.


[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.]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150903/af3235a5/attachment-0001.html>