This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>, Trevor Saunders <tbsaunde at tbsaunde dot org>
- Cc: Jason Merrill <jason at redhat dot com>, Nathan Sidwell <nathan at acm dot org>, Jakub Jelinek <jakub at redhat dot com>, gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 15 Nov 2017 10:33:50 -0500
- Subject: Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes
- Authentication-results: sourceware.org; auth=none
- References: <CADzB+2kfB26fTGXk2AVWuN1mv641OrcEBze+Lq-KexXHeS=Y9A@mail.gmail.com> <1510350329-48956-1-git-send-email-dmalcolm@redhat.com> <1510350329-48956-3-git-send-email-dmalcolm@redhat.com> <20171115061704.q4jypxdhfiofk5o5@ball> <CAFiYyc0hQzkHeMjVicjP-rtMcDvitfUmiWSBTVbeGzOjMR4=Cg@mail.gmail.com>
On Wed, 2017-11-15 at 12:11 +0100, Richard Biener wrote:
> On Wed, Nov 15, 2017 at 7:17 AM, Trevor Saunders <tbsaunde@tbsaunde.o
> rg> wrote:
> > On Fri, Nov 10, 2017 at 04:45:17PM -0500, David Malcolm wrote:
> > > This patch provides a mechanism in tree.c for adding a wrapper
> > > node
> > > for expressing a location_t, for those nodes for which
> > > !CAN_HAVE_LOCATION_P, along with a new method of cp_expr.
> > >
> > > It's called in later patches in the kit via that new method.
> > >
> > > In this version of the patch, I use NON_LVALUE_EXPR for wrapping
> > > constants, and VIEW_CONVERT_EXPR for other nodes.
> > >
> > > I also turned off wrapper nodes for EXCEPTIONAL_CLASS_P, for the
> > > sake
> > > of keeping the patch kit more minimal.
> > >
> > > The patch also adds a STRIP_ANY_LOCATION_WRAPPER macro for
> > > stripping
> > > such nodes, used later on in the patch kit.
> >
> > I happened to start reading this series near the end and was rather
> > confused by this macro since it changes variables in a rather
> > unhygienic
> > way. Did you consider just defining a inline function to return
> > the
> > actual decl? It seems like its not used that often so the slight
> > extra
> > syntax should be that big a deal compared to the explicitness.
>
> Existing practice .... (STRIP_NOPS & friends). I'm fine either way,
> the patch looks good.
>
> Eventually you can simplify things by doing less checking in
> location_wrapper_p, like only checking
>
> +inline bool location_wrapper_p (const_tree exp)
> +{
> + if ((TREE_CODE (exp) == NON_LVALUE_EXPR
> + || (TREE_CODE (exp) == VIEW_CONVERT_EXPR
> + && (TREE_TYPE (exp)
> + == TREE_TYPE (TREE_OPERAND (exp, 0)))
> + return true;
> + return false;
> +}
>
> and renaming to maybe_location_wrapper_p. After all you can't really
> distinguish location wrappers from non-location wrappers? (and why
> would you want to?)
That's the implementation I originally tried.
As noted in an earlier thread about this, the problem I ran into was
(in g++.dg/conversion/reinterpret1.C):
// PR c++/15076
struct Y { Y(int &); };
int v;
Y y1(reinterpret_cast<int>(v)); // { dg-error "" }
where the "reinterpret_cast<int>" has the same type as the VAR_DECL v,
and hence the argument to y1 is a NON_LVALUE_EXPR around a VAR_DECL,
where both have the same type, and hence location_wrapper_p () on the
cast would return true.
Compare with:
Y y1(v);
where the argument "v" with a location wrapper is a VIEW_CONVERT_EXPR
around a VAR_DECL.
With the simpler conditions you suggested above, both are treated as
location wrappers (leading to the dg-error in the test failing),
whereas with the condition in the patch, only the latter is treated as
a location wrapper, and an error is correctly emitted for the dg-error.
Hope this sounds sane. Maybe the function needs a more detailed
comment explaining this?
Thanks
Dave
> Thanks,
> Richard.
>
> > Other than that the series seems reasonable, and I look forward to
> > having wrappers in more places. I seem to remember something I
> > wanted
> > to warn about they would make much easier.
> >
> > Thanks
> >
> > Trev
> >