On Thu, Jan 04, 2024 at 03:30:18PM +0100, Olivier Certner wrote: > Hi Konstantin, > > This is the commit message: > > > > libthr: thr_attr.c: Clarity, whitespace and style > > > > > > Also, remove most comments, which don't add value. > > There was no intention on my side to make any other changes other than mostly > mechanical ones to the existing code in this particular commit, which already > is in itself a big diff.
In the commit, of course. In the flow of making this commit, the separate follow-up that cleans debris is quite reasonable. Also we usually discourage plain style changes or tweaks unless they are preparations for more substantial change. > > > The check is not needed. > > Indeed. It was there from the start. > > > there and in all similar places that test a flag bit > > I have already been told multiple times that testing a flag in an integer > with '&' is to be assimilated as a boolean result, which per style(9) doesn't > require explicit testing for non-zero. I think I even had to change code > once in a review for this reason. Can you show the place where it happens? Because the canonical style is reverse, non-bool values must be explicitly tested instead of relying on the implicit conversion to bool. > > I'm happy to change that if there can be a clear consensus on one way or the > other. > > Additionally, this is not code I touched (except for indentation). > > > > + if ((pattr = malloc(sizeof(*pattr))) == NULL) > > > + return (ENOMEM); > > > + > > > + memcpy(pattr, &_pthread_attr_default, sizeof(struct pthread_attr)); > > Above you changed sizeeof(struct pthread_attr) to sizeof(*pattr), but > > there you left the type name. > > Ah, indeed, thanks for spotting that inconsistency. > > > For me this looks somewhat confusing, in other places the explicit flag > > name symbol is used instead of a variable value. > > With the assignment being in the preceding line, I'm not sure I share that > sentiment, though I agree this is inconsistent with the 'else' branch. Happy > to change it in a subsequent commit (original code is unchanged). > > > There should be a blank line after the local vars declaration. > > Yes, a style-fixup that I missed, thanks. > > > What is the point of doing calloc() if the whole allocated memory is > > overwritten by the memcpy() call below? > > I agree this can be changed (in a subsequent pass, this is not a style issue, > although arguably a clarity one). > > I'll make sure you're added to differential revisions changing this file. > > Thanks and regards. > > -- > Olivier Certner
