Re: separate information from variable value for window goal_column

[Gavin Smith]

On Wed, Oct 09, 2024 at 08:00:42PM +0200, Patrice Dumas wrote:
> Hello,
> 
> Here is a patch that I would like to propose, but it could be
> controversial so I prefer to ask here first.  This is in info code,
> currently window->goal_column is set to -1 to mean that the
> column to place the cursor in when moving it up is the column it is
> currently in.
> 
> I propose in the patch to separate the information set by -1 from the
> goal_column value, by setting this information in the window->flags
> separately from the goal_column.  This is for two reasons, first I think
> that using direclty the variable value to convey another information than
> the value itself (here the column index) leads to code more difficult to
> understand, as when the variable value is accessed (in case of
> comaprisons, in particular) or set, it may not be immediately clear if
> it is to set or access the ancillary information or the main value.

You could view it another way and say when debugging it is harder to
understand when looking at the value of 'goal_column' as you don't
know whether the value there is meaningful without also looking at
the flags.  It's not as easy in a debugger to read the flags as it
is just to look at one value.  Just something to consider.

> The
> second reason is that it may be harder to debug when the value is -1 as
> it could be on purpose or not.

I don't know why it would be -1 by accident.  You mean it takes the value
-1 as the result of a bug in the program?

> Another reason is that it is not
> possible to make the variable unsigned even when it is more logical
> (this is the case here, as the value is used in the LINE_MAP structure).

I thought we agreed in a recent discussion we weren't changing types
to be unsigned?  Just because the value should never be negative doesn't
seem like a good reason to use an unsigned type.  Using an unsigned
type doesn't guarantee that there is not a bug that would make the value
negative were a signed type used.

Anyway, your patch is quite clear and potentially makes the code clearer
as someone reading the code doesn't need to remember what "-1" means so
I am happy if you want to apply it.

> --- a/info/window.c
> +++ b/info/window.c
> @@ -289,11 +289,11 @@ window_make_window (void)
>    window->height = (active_window->height / 2) - 1;
>    window->first_row = active_window->first_row +
>      (active_window->height - window->height);
> -  window->goal_column = -1;
> +  window->goal_column = 0;

It seems nicer having the default value being 0 than -1.  (This would
be done automatically by xzalloc.)

> I would like to do similar changes in other cases if it looks like a
> good idea (for node->nodestart_adjusted and node->nodelen).
> 
> Any comment?

I don't remember the details about what those values are used but
it could potentially have the same benefits.  The flag name could be
more descriptive for someone reading the code.

I remember it is tricky in some code initialising objects where some
values are set to 0 and others are set to -1, such as in info_create_node
in nodes.c:

NODE *
info_create_node (void)
{
  NODE *n = xmalloc (sizeof (NODE));

  n->fullpath = 0;
  n->subfile = 0;
  n->nodename = 0;
  n->contents = 0;
  n->nodelen = -1;
  n->display_pos = 0;
  n->body_start = 0;
  n->flags = 0;
  n->references = 0;
  n->up = 0;
  n->prev = 0;
  n->next = 0;

  return n;
}

This seems hard to get right and would be simpler if everything was set to
0 instead.

I'm not saying there is anything wrong about using special values such
as -1 so we should just consider it on a case-by-case basis.

There is also potentially the issue about running out of bits in an
int for flags.