Re: [PATCH 02/14] Support for adding and stripping location_t wrapper nodes

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Nov 15, 2017 at 4:33 PM, David Malcolm <dmalcolm@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@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?

Yes.  I guess the above would argue for a new tree code but I can
see that it is better to avoid that.

Thanks,
Richard.

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