Re: [PATCH (2) 34/34] unix.7: Use sizeof consistently

[Alejandro Colomar <colomar.6.4.3@gmail.com>]

Hi Michael,

On 9/5/20 10:27 AM, Michael Kerrisk (man-pages) wrote:
> Yes, the threading made things a little tricky, especially when it
> came to trying to review what I'd done. Did you not send with 
> "git send-email"? Usually that threads things nicely (all patches 
> after the first as replies to the first patch).

In this case, I didn't, because the conversation was already started,
even the set of patches was already started, and I had to edit all of
the subjects, and sometimes add introductory messages, and I felt more
comfortable with the GUI.

> So, I've still not processed patches 21, 22, and 29. And in review,
> I see that I am wondering about whether I should maintain 1, 5, 17,
> 18, and 19. These all involve the use of malloc() or similar.
> 
> The existing pattern was something like:
> 
>     struct mytype *x;   // Or some simple type such as 'int'
>     ...
>     x = malloc(n * sizeof(struct mytpe));

Not to forget `malloc(sizeof(struct mytpe) * n);`

> 
> and your patches change it to:
> 
>     struct mytype *x;
>     ...
>     x = malloc(n * sizeof(*x));>
> I'm not sure that always helps readability.
> 
> Part of the problem is the use of C90 in the code.
> 
> Do you both agree with me that both of the following c99
> forms are better than the original:
> 
>     struct mytype *x = malloc(n * sizeof(struct mytpe));
>     struct mytype *x = malloc(n * sizeof(*x));
> 
> ?

Yes, I would say both of these are an improvement.
> 
> I *think* I mildly prefer the first form, but I'm open to
> arguments that the latter form is preferable. Of course, the
> fact that there might be more than one point where an 'alloc'
> is done and assigned to 'x' may influence the argument. Thus
> 
> 
>     struct mytype *x = malloc(n * sizeof(struct mytpe));
>     ...
>     x = malloc(p * sizeof(struct mytype));
> 
> vs
> 
>     struct mytype *x = malloc(n * sizeof(*x));
>     ...
>     x = malloc(p * sizeof(*x));

In case there are 2 or more allocs, in general, I prefer the name of the
variable.

In case there is only 1 alloc in the same line as the declaration, I
still prefer the name of the variable: for consistency, and because some
day you may add another alloc, and then separate the original
declaration+alloc in two lines, and forget to fix sizeof to use the name
of the variable.

The cases where I see the type much better are cases where it is
impossible for the type to change (and if it ever changed it would be an
accident and cause a deserved bug) such as in those cases where you
really need an (u)int64_t because of the API.

There's also cases where in real code I would prefer the name of the
variable (to avoid future bugs because of type change), but in the man
pages it is clearer if you write the type to be more explicit and
consistent.  Example: queue.3 (PATCH 24/34): It's clearer if you
consistently use the type across all the code (and it may be therefore
better to use it in the man-pages), because the name of the variable
looks like it's different from one alloc to the next, but I can imagine
some real code implementing a TAILQ and later deciding to use a CIRCLEQ,
and if any of the types in the allocation are not updated accordingly,
there will appear bugs, while if the name of the node is used for
allocating the memory, the transition will be really simple.

Regards,
Alex.