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