Date: Thu, 04 Jan 2024 15:30:18 +0100 From: Olivier Certner <olce@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: bd61c1e89dc4 - main - libthr: thr_attr.c: Clarity, whitespace and style Message-ID: <2782916.iL6vRArjjl@ravel> In-Reply-To: <ZZacjltT9BQHfMZI@kib.kiev.ua> References: <202401041044.404AiIN0046997@gitrepo.freebsd.org> <ZZacjltT9BQHfMZI@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--nextPart85221458.0ko45tJjV3 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="UTF-8"; protected-headers="v1" From: Olivier Certner <olce@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Date: Thu, 04 Jan 2024 15:30:18 +0100 Message-ID: <2782916.iL6vRArjjl@ravel> In-Reply-To: <ZZacjltT9BQHfMZI@kib.kiev.ua> MIME-Version: 1.0 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 --nextPart85221458.0ko45tJjV3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQIzBAABCQAdFiEEmNCxHjkosai0LYIujKEwQJceJicFAmWWwPoACgkQjKEwQJce JicgkQ//QctR4O6qseGMiofKrhtOSa0NDXzUkp2iHyoxYTz7AnaCQN8P7f2tJB2t kuXXYt8h3Ct+0uulkDnsoy9vaXS1A5kSlQ0+vby30Atgewaw9Tcfm45Pti4euw3j OYmCb3J+OikyRfF5uI7diOcN2DFKm/qRK+5WqkaYBTm9Rc2fKchV34lMZtqBIXUX mSeQSkNVUCOU6b760YHDOCrCfOuV/tQUyB55lYbITBDQZKsvzEUCiJ9D2vdgTvOW CpE+n6bCp9nLuBdO59rd3THNMdjujcxNmNeaxlrliE6pA2LGHiXOyCdQFy6bLccF ZOuY7F5PM9743JCFpkqbiXKJMiiY96lkrVgU0srUbaoAmRlmCFwLpdjzrH/yiLN1 7Vmq+Fk2yuX9AdtyMDAslRpWhSNTC2lajY263IUHF3qcyv5Vg06dkRoEfncKtFFX TIOimWIgjWqEVJoR6t6Klssz92urgW/rPk8bAnNOT9nAQqZCzFpV4U7EXy1ltkE7 W9DJR8XDJ5OkTarDqLPSOK0GVuWx5+r+ni1MQtYBQFF0/Nrds+4wvg86cicPexD8 errksz++WGfj8HMDH+1d25EbGX+pzE+z4z2XJLGKYpy0/S3eL9nmgK5VIcWwc/ky /AA3RMrDYaB66MWEuovuGlxIf72SFx0n6Kq8qW0gecwJQcs17TM= =nVL9 -----END PGP SIGNATURE----- --nextPart85221458.0ko45tJjV3--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2782916.iL6vRArjjl>