Re: [committed][gdb/symtab] Make find_block_in_blockvector more robust

[Tom de Vries <tdevries@suse.de>]

On 10/22/20 11:42 PM, Simon Marchi wrote:
> On 2020-10-22 5:21 p.m., Tom de Vries wrote:
>> On 10/22/20 8:56 PM, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>
>>> Tom> +      if (!(BLOCK_START (b) <= pc))
>>> Tom> +	return NULL;
>>>
>>> This seems a bit weird to me, in that if BLOCK_START(b) == pc, then I
>>> would be inclined to say that the pc is in fact in that block.
>>>
>>
>> So if BLOCK_START(b) == pc, indeed the pc is in the block, and we have:
>> ...
>>       if (!(true))
>> 	return NULL;
>> ...
>> which I'd say correctly handles that case.
>>
>> Thanks,
>> - Tom
>>
> 
> I think that turning:
> 
>   if (!(BLOCK_START (b) <= pc))
> 
> into
> 
>   if (BLOCK_START (b) > pc)
> 
> or
> 
>   if (pc < BLOCK_START (b))
> 
> would make it easier to read.

Thanks for the feedback.

I agree it would make it easier to read, but I'm afraid it would make it
harder to understand.

In general I prefer range tests to conform to this layout:
...
min cmp-op-1 val && val cmp-op-2 max
...
which IMO is the best approximation of the non-C:
...
min cmp-op-1 val cmp-op-2 max
...
and then use the range expression as a whole, either negated or not.

But indeed, the negated case is a bit harder to read, something I
sometimes fix by using a variable with a somewhat helpful name.

This follows my preferred layout for the range expression (albeit split
up into two variables), while not negating non-trivial expressions:
...
  while (bot >= STATIC_BLOCK)
    {
      b = BLOCKVECTOR_BLOCK (bl, bot);
      bool start_ok = BLOCK_START (b) <= pc;
      bool end_ok = pc < BLOCK_END (b);
      if (!start_ok)
        return NULL;
      if (end_ok)
        return b;
      bot--;
    }
...
but perhaps this is making things less clear?  WDYT?

Thanks,
- Tom