Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: PATCH: Cleanup isarr fields and variables



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.

I agree that at params.c:2196, v has been zeroed out. No explicit zero assignments are technically needed. Currently flags is explicitly zeroed out but not isarr and arr. I find this a little confusing. We should either have explicit assignments for all of the fields that are meant to be zeroed out or none of them.

If we decide to go with no explicit zero assignments, I would rewrite the code just above as follows:

if (!v)
    v = (Value) zhalloc(sizeof *v);
memset(v, 0, sizeof(*v));

This way it's in my opinion more obvious that v is always zeroed out. The same could be done at params.c:2223.

In other places, like glob.c:1724, the explicit zero assignments should be kept (or a call to memset should be added) since in C values allocated on the stack aren't guaranteed to be zeroed out.

Philippe



On Fri, Nov 21, 2025 at 10:33 PM Oliver Kiddle <opk@xxxxxxx> wrote:
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



Messages sorted by: Reverse Date, Date, Thread, Author