From owner-svn-src-all@freebsd.org Sun May 19 15:55:51 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C68A715B15AF; Sun, 19 May 2019 15:55:51 +0000 (UTC) (envelope-from ota@j.email.ne.jp) Received: from mail03.asahi-net.or.jp (mail03.asahi-net.or.jp [202.224.55.15]) by mx1.freebsd.org (Postfix) with ESMTP id 17B6F6A501; Sun, 19 May 2019 15:55:49 +0000 (UTC) (envelope-from ota@j.email.ne.jp) Received: from vmware.advok.com (pool-72-76-119-135.nwrknj.fios.verizon.net [72.76.119.135]) (Authenticated sender: NR2Y-OOT) by mail03.asahi-net.or.jp (Postfix) with ESMTPSA id 05A1B43A04; Mon, 20 May 2019 00:55:45 +0900 (JST) Date: Sun, 19 May 2019 11:54:56 -0400 From: Yoshihiro Ota To: rgrimes@freebsd.org Cc: "Rodney W. Grimes" , Benedict Reuschling , src-committers@freebsd.org, svn-src-stable@freebsd.org, svn-src-all@freebsd.org, svn-src-stable-12@freebsd.org, Bruce Evans , Konstantin Belousov Subject: Re: svn commit: r347951 - stable/12/lib/libc/stdlib Message-Id: <20190519115456.40571481c85c9f178657d6b9@j.email.ne.jp> In-Reply-To: <201905181306.x4ID6piX064982@gndrsh.dnsmgr.net> References: <201905181306.x4ID6piX064982@gndrsh.dnsmgr.net> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; i386-portbld-freebsd12.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 17B6F6A501 X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; spf=pass (mx1.freebsd.org: domain of ota@j.email.ne.jp designates 202.224.55.15 as permitted sender) smtp.mailfrom=ota@j.email.ne.jp X-Spamd-Result: default: False [-0.79 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; RCVD_IN_DNSWL_LOW(-0.10)[15.55.224.202.list.dnswl.org : 127.0.5.1]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:202.224.55.0/24]; MV_CASE(0.50)[]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[email.ne.jp]; NEURAL_HAM_LONG(-0.98)[-0.980,0]; NEURAL_SPAM_SHORT(0.49)[0.491,0]; RECEIVED_SPAMHAUS_PBL(0.00)[135.119.76.72.zen.spamhaus.org : 127.0.0.10]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MX_GOOD(-0.01)[sbmx.asahi-net.or.jp]; RCPT_COUNT_SEVEN(0.00)[9]; NEURAL_HAM_MEDIUM(-0.57)[-0.575,0]; IP_SCORE(0.08)[asn: 4685(0.47), country: JP(-0.05)]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:4685, ipnet:202.224.32.0/19, country:JP]; MID_RHS_MATCH_FROM(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 May 2019 15:55:52 -0000 I wonder if we can use a tool to confirm coding style like clang-format or something else. I tried to setup clang-format and 2 others in the past like a year ago but wasn't able to do... Hiro On Sat, 18 May 2019 06:06:51 -0700 (PDT) "Rodney W. Grimes" wrote: > > Hello Konstantin and Bruce, > > > > thank you for your comments. Clearly, this should have been addressed in > > the review and checked by a second pair of eyes. I mostly did the man > > page changes and can't comment much on the actual coding example. Two > > people (dab and jilles) approved the revision in the Phabricator review. > > There was plenty of time to review it between April 16 and May 15 when > > then the original submitter asked if there was anything else that needs > > to be done. > > The quality of a code review is highly dependent on who is invited > to do the reviewing, and how much participation and care those invited > have with respect to the process. Man pages that contain code samples > need to have more reviewers, you need man page experts, you need subject > mater experts, and you need coding experts. This review could of used > more reviewers (hind sight is 20/20) but if they had not been the right > reviewers the low quality commit would of probably still occurred. > > The amount of time in review is not a good measure of review completness, > that falls more onto the quality of the workmanship of those actually > commented on a review. I myself consider a LGTM, or a accept by a reviewer > who makes 0 comments on my code in phab as if that reviewer probably > just rubber stamped my change, UNLESS I have extremly high trust in > that person. If bde, imp, or kib stamp an approve on something of mine my > confidence level is very high and I do consider that code well reviewed, > if I get a rubber stamp from some others I am not so confident. > > Now we have the problem in that note every code review can be done by > such high quality engineers due to time and work load constraints, so > we need to raise the level of others closer to these shinning stars > and offset this lack of avaliable resources by other means (more > reviewers, long review cycles, automations, etc.) > > > > Now, having said that and the the change is now both in HEAD and > > stable/12, how do we proceed? Revert them both or patch those issues as > > follow-ups? Note that a src committer should do that and not me trying > > to add more EXAMPLE sections to man pages with my doc commit bit. > > There is another option, a revert is actually a local operation > to your checked out code, ou can check out head, revert this > commit, but do not commit, make the corrections, and then commit. > > When MFC's this single commit does the right thing. > > You could also revert in head, commit and later fix these > issues, commit, and then MFC both of those commits. > > All of your proposed solutions, and my additions are workable, > and I really have no preference on a single file change like > this. > > Good Day, > Rod > > > Regards, > > Benedict > > > > > > > > Am 18.05.19 um 07:45 schrieb Bruce Evans: > > > On Sat, 18 May 2019, Konstantin Belousov wrote: > > > > > >> On Sat, May 18, 2019 at 03:15:08AM +0000, Benedict Reuschling wrote: > > >>> Author: bcr (doc committer) > > >>> Date: Sat May 18 03:15:07 2019 > > >>> New Revision: 347951 > > >>> URL: https://svnweb.freebsd.org/changeset/base/347951 > > >>> > > >>> Log: > > >>> ? MFC r347617: > > >>> ? Add small EXAMPLE section to bsearch.3. > > >>> > > >>> ? Submitted by:??????????? fernape (via Phabricator) > > >>> ? Reviewed by:??????????? bcr, jilles, dab > > >>> ? Approved by:??????????? bcr (man pages), jilles (src) > > >>> ? Differential Revision:??????? https://reviews.freebsd.org/D19902 > > >>> > > >>> Modified: > > >>> ? stable/12/lib/libc/stdlib/bsearch.3 > > >>> Directory Properties: > > >>> ? stable/12/?? (props changed) > > >>> > > >>> Modified: stable/12/lib/libc/stdlib/bsearch.3 > > >>> ============================================================================== > > >>> > > >>> --- stable/12/lib/libc/stdlib/bsearch.3??? Sat May 18 02:02:14 > > >>> 2019??? (r347950) > > >>> +++ stable/12/lib/libc/stdlib/bsearch.3??? Sat May 18 03:15:07 > > >>> 2019??? (r347951) > > >>> @@ -32,7 +32,7 @@ > > >>> ?.\"???? @(#)bsearch.3??? 8.3 (Berkeley) 4/19/94 > > >>> ?.\" $FreeBSD$ > > >>> ?.\" > > >>> -.Dd February 22, 2013 > > >>> +.Dd May 15, 2019 > > >>> ?.Dt BSEARCH 3 > > >>> ?.Os > > >>> ?.Sh NAME > > >>> @@ -83,6 +83,61 @@ The > > >>> ?function returns a pointer to a matching member of the array, or a null > > >>> ?pointer if no match is found. > > >>> ?If two members compare as equal, which member is matched is > > >>> unspecified. > > >>> +.Sh EXAMPLES > > >>> +A sample program that searches people by age in a sorted array: > > >>> +.Bd -literal > > >>> +#include > > >>> +#include > > >>> +#include > > >>> +#include > > >>> +#include > > >>> + > > >>> +struct person { > > >>> +??? char name[5]; > > >>> +??? int age; > > >>> +}; > > > > > > This example has a high density of style bugs.? kib pointed out some. > > > Some of the others are: > > > > > > (1) Not sorting the struct members on alignment.? (style(9) says to sort > > > ??? on use, then size, but means alignment). > > > (2) Not indenting the struct member names. > > > (3) Not use a prefix for struct member names.? This is not important for > > > ??? small programs. > > > > > >>> + > > >>> +int > > >>> +compare(const void *key, const void *array_member) > > >>> +{ > > >>> +??? int age = (intptr_t) key; > > >>> +??? struct person person = *(struct person *) array_member; > > >> These two lines contain at least three style(9) bugs, and at least one > > >> warning at higher warning level. > > >> > > >>> + > > >>> +??? return (age - person.age); > > >>> +} > > >>> + > > >>> +int > > >>> +main() > > >> Why use K&R definition ? > > >> > > >>> +{ > > >>> +??? struct person *friend; > > >>> + > > >>> +??? /* Sorted array */ > > >>> +??? struct person friends[6] = { > > >>> +??????? { "paul", 22 }, > > >>> +??????? { "anne", 25 }, > > >>> +??????? { "fred", 25 }, > > >>> +??????? { "mary", 27 }, > > >>> +??????? { "mark", 35 }, > > >>> +??????? { "bill", 50 } > > >>> +??? }; > > >>> + > > >>> +??? size_t array_size = sizeof(friends) / sizeof(struct person); > > >> Since you used const elsewere, why did not you used it there ? > > > > > > (4) The size expression is obfuscated by spelling the element size as > > > ??? sizeof(struct person) instead of sizeof(friends[0]). > > > (5?) FreeBSD now spells this size expression as nitems(friends).? I don't > > > ??? like the unportability of this, especially in an example. > > > > > >> > > >>> + > > >>> +??? friend = bsearch((void *)22, &friends, array_size, sizeof(struct > > >>> person), compare); > > >> Taking address of an array is weird. > > >> Line is too long. > > > > > > (6) C99 specifies that the search is for a member of the array that matches > > > ??? the object pointed to by 'key'.? Here the key of (void *)22 is a > > > ??? not a pointer to an object.? It is a cookie which points to garbage. > > > ??? The cookie is unique enough to work in practice.? Perhaps you can > > > ??? prove it to always work if the option type intptr_t is supported. > > > ??? But this is a bad example. > > > (4a) same obfuscation of the element size. > > > > > >> > > >>> +??? assert(strcmp(friend->name, "paul") == 0); > > >>> +??? printf("name: %s\enage: %d\en", friend->name, friend->age); > > >>> + > > > > > > (7) Extra blank line.? This is not too bad, but is not done consistently. > > > > > >>> +??? friend = bsearch((void *)25, &friends, array_size, sizeof(struct > > >>> person), compare); > > > > > > (8) More too-long lines. > > > (4b) More sizeof(person)'s. > > > > > >>> +??? assert(strcmp(friend->name, "fred") == 0 || strcmp(friend->name, > > >>> "anne") == 0); > > > > > > (7a) No extra blank line for "fred". > > > > > >>> +??? printf("name: %s\enage: %d\en", friend->name, friend->age); > > >>> + > > >>> +??? friend = bsearch((void *)30, &friends, array_size, sizeof(struct > > >>> person), compare); > > >>> +??? assert(friend == NULL); > > >>> +??? printf("friend aged 30 not found\en"); > > > > > > I didn't notice before that the key cookie was an encoding of the age. > > > > > > The key is not unique (there are 2 25's).? This gives an example of low > > > quality data and the example has minimal error handling handling for this > > > without describing what it is doing (it asserts uniqueness for age 22 and > > > it asserts one of the 2 possibilities for age 25.? It can only do this > > > because it knows too much about the data). > > > > > > It is more than a style bug to handle errors in data by assert() or > > > otherwise killing the program, except possibly when the data is supposed > > > to be good. > > > > > >>> + > > >>> +??? return (EXIT_SUCCESS); > > >>> +} > > >>> +.Ed > > >>> ?.Sh SEE ALSO > > >>> ?.Xr db 3 , > > >>> ?.Xr lsearch 3 , > > > > > > Bruce > > > > > > -- > Rod Grimes rgrimes@freebsd.org > _______________________________________________ > svn-src-all@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/svn-src-all > To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"