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.
