From nobody Fri Jan 05 05:19:48 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4T5sGN6mmPz56Klj; Fri, 5 Jan 2024 05:19:56 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4T5sGN3dznz4bqv; Fri, 5 Jan 2024 05:19:56 +0000 (UTC) (envelope-from kostikbel@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: from tom.home (kib@localhost [127.0.0.1] (may be forged)) by kib.kiev.ua (8.17.1/8.17.1) with ESMTP id 4055JmB6015887; Fri, 5 Jan 2024 07:19:51 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 4055JmB6015887 Received: (from kostik@localhost) by tom.home (8.17.1/8.17.1/Submit) id 4055Jm7C015886; Fri, 5 Jan 2024 07:19:48 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 5 Jan 2024 07:19:48 +0200 From: Konstantin Belousov To: Olivier Certner 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: References: <202401041044.404AiIN0046997@gitrepo.freebsd.org> <2782916.iL6vRArjjl@ravel> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2782916.iL6vRArjjl@ravel> X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=4.0.0 X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-14) on tom.home X-Rspamd-Queue-Id: 4T5sGN3dznz4bqv X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US] 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