On Thu, Nov 30, 2017 at 12:16 PM, David Malcolm <dmalc...@redhat.com> wrote:
> On Thu, 2017-11-16 at 10:58 +0100, Richard Biener wrote:
>> On Wed, Nov 15, 2017 at 4:33 PM, David Malcolm <dmalc...@redhat.com>
>> wrote:
>> > On Wed, 2017-11-15 at 12:11 +0100, Richard Biener wrote:
>> > > On Wed, Nov 15, 2017 at 7:17 AM, Trevor Saunders <tbsaunde@tbsaun
>> > > de.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.
Note that STRIP_NOPS is now implemented in terms of such an inline
function, it would make sense to do the same here.
>> > > 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?
>>
>> Yes. I guess the above would argue for a new tree code but I can
>> see that it is better to avoid that.
Basically, we can only strip a NON_LVALUE_EXPR if the argument is
already not an lvalue. We shouldn't need any extra check on
VIEW_CONVERT_EXPR.
> [...]
>
> Here's an updated version of the patch which:
> * adds a much more detailed comment to location_wrapper_p,
> * fixes an erroneous reference to LOCATION_WRAPPER_EXPR in the
> comment to maybe_wrap_with_location (from an earlier unfinished
> experiment)
> * adds a selftest for handling wrapper nodes.
>
> Is there consensus about whether this approach is sane? i.e.
> * adding wrapper nodes via re-using existing tree codes (this kit), vs
> * adding them via some new tree code ("LOCATION_WRAPPER_EXPR"?), vs
> * the workaround of:
> "[PATCH] C++: use an optional vec<location_t> for callsites"
> https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01392.html
>
> Jason? Jakub? others?
I still like this approach.
Jason
