Date: Sun, 19 May 2019 11:54:56 -0400 From: Yoshihiro Ota <ota@j.email.ne.jp> To: rgrimes@freebsd.org Cc: "Rodney W. Grimes" <freebsd@gndrsh.dnsmgr.net>, Benedict Reuschling <bcr@freebsd.org>, src-committers@freebsd.org, svn-src-stable@freebsd.org, svn-src-all@freebsd.org, svn-src-stable-12@freebsd.org, Bruce Evans <brde@optusnet.com.au>, Konstantin Belousov <kostikbel@gmail.com> 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: <a10ebbe0-120d-a0e3-ef85-74c7c1267170@FreeBSD.org> <201905181306.x4ID6piX064982@gndrsh.dnsmgr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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" <freebsd@gndrsh.dnsmgr.net> 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 <assert.h> > > >>> +#include <stdint.h> > > >>> +#include <stdio.h> > > >>> +#include <stdlib.h> > > >>> +#include <string.h> > > >>> + > > >>> +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"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190519115456.40571481c85c9f178657d6b9>