[PATCH] fix parentheses location in a CXXConstructExpr

[Richard Smith via cfe-commits]

On Thu, Sep 3, 2015 at 2:26 PM, Olivier Goffart <ogoffart at kde.org> wrote:

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


There are also many cases where it doesn't involve parens =)

The problem is that for a declaration of the form:

  T x(y);

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

Conversely, for a declaration of the form:

  T x{y};

... the braces are owned by the initialization expression, not by the
VarDecl, either as an InitListExpr or by the CXXConstructExpr representing
the list initialization.

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.

(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.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150903/5da0c870/attachment.html>