[PATCH] fix parentheses location in a CXXConstructExpr

[Olivier Goffart via cfe-commits]

On Thursday 3. September 2015 13:41:12 Richard Smith wrote:
> 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.


Thanks.

So no dyn_cast_or_null?   CXXBindTemporaryExpr::getSubExpr never returns null 
here?

> [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.]

[This is however quite convnient for me to get the location of the parentheses 
of a CXXConstructorExpr. There are may cases where CXXConstructorExpr is not 
in a CXXFunctionalCastExpr]

-- 
Olivier