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

[Simon Marchi <simark@simark.ca>]

On 2020-10-23 10:08 a.m., Tom de Vries wrote:
> 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.

I think we all picture things a bit differently in our minds, which
means we prefer some form over the other, and that's fine.

Not trying to convince you to change anything, but just for the sake of
discussion, this is how I picture it.  When reading conditions, I
translate to natural language in my head.  Between:

  if the pc is smaller than the start of the block, then abort

and

  if the start of the block is not smaller or equal than the pc

In the first case, I understand in an instant "ok, it means if the pc is
before the range". For the second, it takes me a little pause: "ok, so
if the start of the block is smaller or equal to the pc, it means we're
in range, so if _not_ that, it means we're out of range.".

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

I like this version with variable names.  The variable names convey some
information about the intention of the code, and act as some kind of
"checkpoints".  If you understand what these boolean variables are meant
to represent (hence the name must be clear), you don't need to parse the
condition themselves to understand the algorithm.  If you have some
reason to believe the conditions are erroneous, then you can dig into
them, but otherwise you can skip them and save some mental cycles.  Of
course, it gets more valuable as the size/complexity of the conditions
increase.

Simon