On Thursday 3. September 2015 13:41:12 Richard Smith wrote:
> On Thu, Sep 3, 2015 at 8:58 AM, Manuel Klimek <kli...@google.com> wrote:
> > On Tue, Sep 1, 2015 at 12:31 AM Olivier Goffart <ogoff...@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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
