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

From: Olivier Certner <olce_at_freebsd.org>
Date: Thu, 04 Jan 2024 14:30:18 UTC
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.

> 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.

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