Re: [PATCH 1/3] New function value_has_address

["Ulrich Weigand" <uweigand@de.ibm.com>]

Yao Qi wrote:
> On Tue, Nov 22, 2016 at 07:16:15PM +0100, Ulrich Weigand wrote:
> > > I think that it's good that the names match.  If one is renamed,
> > > so should the other, IMO.  Maybe call the function
> > > value_has_address_location?  I think it'd be good if the
> > > function's intro comment made this link more explicit.
> > > Actually, I see now that patch #3 tweaks the comment.
> > 
> > I think part of the confusion is that the comment above is simply
> > no longer true; for lval_register values, address is *not* (any longer)
> > used to hold any byte offset into a register structure, as far as I
> > can see.  Instead, for lval_register values, the register that holds
> > the value is identifed solely via the VALUE_REGNUM/VALUE_NEXT_FRAME_ID
> > fields, and the address field is ignored.
> 
> That is what I am saying in the last paragraph of cover letter.

I see, I missed that.  I think given that we really only ever should
use value.location.address on lval_memory values, maybe the clearest way
would be to guard accesses with an explicit VALUE_LVAL == lval_memory
check instead of moving that check to a function.  Then we don't have
to debate how that function should be called either :-)

> > I think we should reword the comments to reflect the fact that
> > "address" is only used for lval_address.  On the other hand,
> > the regnum/frame_id fields should move into the union and only
> > be used for lval_register values ...
> > 
> 
> That is what I am trying to do in next step.  Let me finish it and
> include it in this series as well.

Very good, thanks!

[ B.t.w. I noticed one more case of fields that probably ought to
move into the location union: value.parent, .bitsize, and .bitpos
are only ever used for bitfields.   Maybe we should create a new
lval_component (possibly renamed from lval_internalval_component)
that uses those three fields as location; where all further data
is retrieved from the parent value.  But that's certainly another
independent change ... ]

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com