[1/3] New function value_has_address

  This patch is to move duplicated condition checking in three functions
to a single new function, value_has_address.

gdb:

2016-10-27  Yao Qi  <yao.qi@linaro.org>

	* value.c (value_has_address): New function.
	(value_address): Call value_has_address.
	(value_raw_address): Likewise.
	(set_value_address): Likewise.
---
 gdb/value.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
  

Hey Yao,

> +/* Return true if VALUE has address, otherwise return false.  */
> +
> +static int
> +value_has_address (const struct value *value)
> +{
> +  return (value->lval != lval_internalvar
> +	  && value->lval != lval_internalvar_component
> +	  && value->lval != lval_xcallable);

I'm wondering about the function's name. Does a value that
lives in a register, for instance, really have an address?
For me, if there was a function value_has_address, it would
return nonzero only for lval_memory. I'm not too sure if
lval_computed would qualify or not.

Perhaps, what you were looking for, is something like
value_lives_in_inferior?
  

On 11/22/2016 04:50 PM, Joel Brobecker wrote:
> Hey Yao,
> 
>> +/* Return true if VALUE has address, otherwise return false.  */
>> +
>> +static int
>> +value_has_address (const struct value *value)
>> +{
>> +  return (value->lval != lval_internalvar
>> +	  && value->lval != lval_internalvar_component
>> +	  && value->lval != lval_xcallable);
> 
> I'm wondering about the function's name. Does a value that
> lives in a register, for instance, really have an address?
> For me, if there was a function value_has_address, it would
> return nonzero only for lval_memory. I'm not too sure if
> lval_computed would qualify or not.
> 
> Perhaps, what you were looking for, is something like
> value_lives_in_inferior?

The intention of the function is to return true if the value
uses the value.location.address union field as location:

  /* Location of value (if lval).  */
  union
  {
    /* If lval == lval_memory, this is the address in the inferior.
       If lval == lval_register, this is the byte offset into the
       registers structure.  */
    CORE_ADDR address;
...
  } location;

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.

Thanks,
Pedro Alves
  

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