Re: [unavailable values part 1, 01/17] base support for unavailable value contents

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Thu, 10 Feb 2011 20:30:26 +0100, Pedro Alves wrote:
> +/* Defines an [OFFSET, OFFSET + LENGTH) range.  */
> +
> +struct range
> +{
> +  /* Lowest offset in the range.  */
> +  int offset;
> +
> +  /* Length of the range.  */
> +  int length;
> +};

I would find [LOW, HIGH) fields more readable as the code now still calculates
offset + length back and forth all over the code.  FYI, not trying to require
it, though.


> +/* Returns true if RANGES contains any range that overlaps [OFFSET,
> +   OFFSET+LENGTH).  */
> +
> +static int
> +ranges_contain_p (VEC(range_s) *ranges, int offset, int length)

Couldn't even this function stick with the `overlap' term?

`contain' associates to me:
  Returns true if each byte of [OFFSET, OFFSET+LENGTH) is overlapping with any
  range in the RANGES list.

(My English association may not be right, though.)


> @@ -206,6 +310,11 @@ struct value
> +  /* Unavailable ranges in CONTENTS.  We mark unavailable ranges,
> +     rather than available, since the common and default case is for a
> +     value to be available.  This is filled in at value read time.  */
> +  VEC(range_s) *unavailable;

Was there considered the opposite way to have a list of available ranges?

Besides cleaning up the inversion code implemented by this patchset in
read_value_memory it would also enable storing discontiguous memory with
a value.

Currently if you store inferior C++ object into a $convenience_variable you
cannot do much with it as it can no longer read the virtual method table
- besides it may no longer exist in the inferior the current code will not
even try to read it from the inferior.

I faced it also with archer-jankratochvil-vla - if you have a very large
inferior array printing only some slices/subsets of it - you still have to
store for $convenience_variable the whole range between first byte and last
byte accessed, despite most of the ranges in between get never accessed.  You
will mostly print some slices/subsets because the whole array is too large.

So I was considering to turn value->contents into some discontiguous ranges
and this patch could also benefit from it.


>  };
>  
> +int
> +value_bytes_available (const struct value *value, int offset, int length)

ctags will never find a comment defined in a .h file.  I would prefer at least
a stub comment:

/* Function comment present at the declaration.  */

Many legacy functions just do not have any comment so one just does not search
more for a comment when there isn't any shown on the ctags-jump.



> +  i = VEC_lower_bound (range_s, value->unavailable, &newr, range_lessthan);
> +  if (i > 0)
> +    {
> +      struct range *bef = VEC_index (range_s, value->unavailable, i - i);

While this patch revisiou fixed two bugs
it has introduced a new bug - "i - i" -> "i - 1".


Thanks,
Jan