Re: git: bd61c1e89dc4 - main - libthr: thr_attr.c: Clarity, whitespace and style

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