On Thu, Oct 10, 2024 at 04:29:21PM +0100, Gavin Smith wrote:
> > 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.
Indeed, you are right, I didn't consider that, mainly because I seldom
use a debugger, I prefer changing the code to print values...
> > 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?
My point is that it a -1 could be because of a bug or on purpose (to
mean that the column should be the current column in that case).
> > 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?
I try to make variables long if possible, but not when it is compared to
size_t and there is a risk that the comparison is problematic.
> 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.
Indeed, but in that case, there is already a comparison with unsigned
type, so either all the variables should be changed to signed or this
specific variable should be unsigned. It may be better to change all
the variables to be signed (in that case it would mean changing LINE_MAP
size and used to be long, but in that precise case, I thought that it
would be too much change. Also in that precise case, it seems
completely impossible for the variable to become negative as it is only
set to values that are either 0 or only incremented.
> 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.
Ok, will do.
> 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.
That's exactly what I could change next, without changing the type in
these cases.
> 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.
Indeed. I think that it is ok as find_node_separator and similar
functions.
--
Pat
