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

[David Malcolm <dmalcolm@redhat.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
> >