Re: PATCH: Cleanup isarr fields and variables

[Oliver Kiddle <opk@xxxxxxx>]

Bart Schaefer wrote:
> Just reading your descriptions without looking at code yet, this seems
> reasonable, but also seems like the sort of thing I'd rather put off
> until the other threads we've been pursuing have been put to rest.

I've just gone through this in some detail to satisfy myself that these
patches don't affect anything negatively and help with the clarity of
the code. My conclusion is positive on both counts.

Is it just the reviewing effort that you want to put off? If there
aren't other factors like the field renames breaking incomplete work
then I'm willing to vouch for this and perhaps commit it. Or I can leave
it with you if you prefer?

The one patch that I would question is 5 (init-all-value-fields).
Mainly the part after params.c:2196. We start by doing memset(v, 0,
sizeof(*v)); An alternate branch does hcalloc() but that also does a
similar memset(). So everything is already zeroed and we could sooner
remove a line. In general, I don't really like explicit zero assignments
that aren't needed by the code's logic because their presence implies
that they are needed, adding noise to the process of trying to follow
the logic. I'm not oblivious to the reasons why it has become common
practice, however. A memset() somehow better communicates that it is
being wiped to be on the safe side.

The original value for SCANPM_ISVAR_AT reminds me that unsigned ints
can be a simpler choice for bit fields - no need to worry about sign
extension and if you need to set the top bit, it is the same as any
other.

Oliver