From owner-svn-src-stable-12@freebsd.org Sat May 18 12:39:11 2019 Return-Path: Delivered-To: svn-src-stable-12@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 7C7D615B1AF3; Sat, 18 May 2019 12:39:11 +0000 (UTC) (envelope-from bcr@FreeBSD.org) Received: from mxout1bln1.prossl.de (mxout1bln1.prossl.de [91.233.87.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DEBA8802E9; Sat, 18 May 2019 12:39:10 +0000 (UTC) (envelope-from bcr@FreeBSD.org) Received: from Voyager-109.local ([207.107.70.114]) (authenticated bits=0) by mx1bln1.prossl.de (8.15.2/8.14.9) with ESMTPSA id x4ICd5sn053472 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 18 May 2019 14:39:06 +0200 To: Bruce Evans , Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org References: <201905180315.x4I3F8g2080553@repo.freebsd.org> <20190518082838.GW2748@kib.kiev.ua> <20190518210514.V913@besplex.bde.org> From: Benedict Reuschling Openpgp: preference=signencrypt Autocrypt: addr=bcr@FreeBSD.org; prefer-encrypt=mutual; keydata= mQENBFr4feYBCACdrnRpuvW/d/PeKuFu54ifaGhG+yFf3wnWaQX8hTCvySbv6A24Owot6cyU vhjdsifZ9iRQ82Likl+6OxI9qBXR9N+WCv1ut46q+mL50YLnYRejRCz8vEmVunlyVDKjsBY5 DtjsaRXMJ/D56wh3ROd9VYjrDHWobu+bg9D7RIv0kCyoPd0JsYRFXISgt4IocWVLT8ciWqiT Pp3m37BPrtXhR3EDOnHmGWPf6SuolvveqyOlpdguD3LAkFJDLeq3isnMaLXUhcsTqBTBLdOT 6EJHBYCcq39izNVC88JsVBmEuByhpb0LaXrBFQWhUG5RyecROAqrx+DWhcZnxayCtr3NABEB AAG0N0JlbmVkaWN0IFJldXNjaGxpbmcgKHd3dy5mcmVlYnNkLm9yZykgPGJjckBGcmVlQlNE Lm9yZz6JAVQEEwEKAD4CGwMFCwkIBwMFFQoJCAsFFgIDAQACHgECF4AWIQRTQnZtbeuE5dPj 3Pah0MHTBYXRWAUCWviOpwUJA8J3wQAKCRCh0MHTBYXRWJfJB/9qxBylB2KlA5EkdhyOp9cO O8kGpx301Rg6I/IUDACmDRvZckthpaUlNJ/RBYtid12o9A9kIhabdEaZfoBcV+ehyq3iSnff 9U+YlXkQ0iOflujWfgyMKNmaseWAPl6aCYc4gUsRH6YTVJFANPdofAIghEFLeY4jPsfkCIPP SAzUK1kQDtsHc5zwTYltqiKRBxQe1Y8WftitRp4LkTkR2wxWggp5oS6yKzh0kRir+U+5pFWZ 9g4ZQGxqodp8fKHI1IsJvs921UArjbmtLmDbrd28SK96i5ck4xga6mPf7ShhE9haYy1A+cbK fDhwizaJUl/H3jjXXbPv1MuIsvo+qEKVuQENBFr4f7wBCACxULIM5UFaEyZwAojYMXwIF/QW zq20MXaQXjn2JxJp04VaQHCL794ZrNtvkTvyqxYVFCKjvJHPXsT4zKuglMSTi83choejs7DJ 4YNmBBJKDHbugAcb5OgdTaT+ztUGl1VL4S3iEvWJwBAOUuzd9TTC/4GhYnUJR7A0CeEZnPfF av2K+d8BZ8x/XryFgQo3Wi5FnOekwls4v1wzzJE4ssaW4p357fmXRG2Czzytzf/W3I2/VWFm 2J8P8TIzSoJq+girktqhH+dYwbDeWkldWj6egcMdpVzJ89ottXARQYeu6YFSHnmHHnBhX2hV z+K2WkVLqYwj7XdBQAtPxIk8VlMBABEBAAGJAnIEGAEKACYCGwIWIQRTQnZtbeuE5dPj3Pah 0MHTBYXRWAUCWviOsgUJA8J19gFAwHQgBBkBCgAdFiEEwXonQDvNfP/33IoBVXQ7/QHhjTUF Alr4f7wACgkQVXQ7/QHhjTWg7Qf/WzBvcfskO8HBHfmLEE5zJRIOgogl6kVwvwR6PjihhMFC iBeH2fGz8nTtkouSttcbMnLnmV0qVv/r5FgIohTlEimaQUh7LgQKiql2SdYVpF1ha/3NHerR 1Rc0OGW+eliseI9b9/OKkY1AOaRM98HGJZB6TO2iKf2wiJDjr1RdlN2VcCST+ksG2Ehc3RvZ UTY1Kw5i9BOKsTsiQkrpCsrZ2/0tqvwv7efYJ0FOHfm0WbmvAlfdpxqsWmtgdsa0m/ItBtpo zIhlZs8/BlOmDK5571bAmWOV0apXSgp6OTmeQkpIsJKjwj/JwOhzvfkxZv03Js4+51xKjotT ma8XGjnJKgkQodDB0wWF0Vj+Wwf/f5cEjvLmq+Xw0KYjPA8oqk9L5a4Z9J/BdeH630koZ2wP YUwUsF/rFfdZf7jHvJJas1I0ctsutvhlgu4Elb7kJrfNRMSXUXe8ucdPS/cH9M/y9e7flW3i c/Ik14rV1Z1E9ME0CtDzfUtS3BfhyxpgmHbQmgPv/NkCE8Zu+nU/1QInpi7ZrDWWyJOCndcm R4kZpP0MJ0OCTWGDkDtJG4zIRQKeJKscqolWo7yu3GiG/Fr9W5cfr3IIo9xp8UQ06idbjjSV jItrSmL/HoZYQ7DSGCsQObC6snbkdeWRSaz/TCzEth5O0NGgA37apNaF3TZTbxRWcMG9uICc oGpDIZeCe7kBDQRa+IBQAQgA2EV0IFUgcJEpznzSxRhKajgLUmP/CJkUrXRipRrR0PqeXEH2 dF1+T8JObDVBN33oXwbXfIvBUhEw2uWsAHDW8OqzUsCTUxdy3ehVHkxHw1deXfvYf5VTtSli QsVIEJ0LZtD3V0idDnfGAhWqbMubBtF2tj1I6P2Py+RlU1pMD4B4g8zcWU+H4H39tpLkd9c/ kTemaX6QwRNW42L8+tDjL4pUogf8/tweMOj9LDOWVTqE2lipWxCH0uTEj8G8kSMLCyGgjxni MFDpjJLu3ETBZMevA+HNWS80RGbT2byu0FbeqXdRV3/+PL3MUY3mOs91bVgxpULG3aDcEmWd IJzNgQARAQABiQE8BBgBCgAmAhsMFiEEU0J2bW3rhOXT49z2odDB0wWF0VgFAlr4jsAFCQPC dXAACgkQodDB0wWF0VgnUQf/T1Z0SBj8YdepF91TTQnvH21USuyEsWCIX8d2xrUqGdVnwlIS fllS3SKTZQJdFhqQtEEYrKRhUDHrPt5Sm0NBisiD+lLcRajv7si4Xj8/ZY/gFjt7RrraYv5D Yb1BjSzsY4YEtq55jDknGufmBpannhnLFFlltt12Sa+xVgZMOl3g8zMjQkiPMtesw1DMDy9N lPB2WlbBVQlkzNHMifpIEiqc+ckZJavYabl/Nsv/kbMFGTizdIllN2EML6l9/KUC3Iw0OWB9 pnge7j3cX7+6sv4ypu65B/XMb4h/4HvXs4D2NW0HIIWtPA50fjjPSJs2JHJt9qVkI7/rNNNL byRcKQ== Organization: The FreeBSD Project Subject: Re: svn commit: r347951 - stable/12/lib/libc/stdlib Message-ID: Date: Sat, 18 May 2019 08:39:03 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190518210514.V913@besplex.bde.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="fsnKyQeONjPYq8m4rld7XVtjUBPd0qtcy" X-Null-Tag: bfeeb3f8352a17fade0e257ec4538b82 X-Rspamd-Queue-Id: DEBA8802E9 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.95 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; NEURAL_HAM_SHORT(-0.95)[-0.952,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 May 2019 12:39:11 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --fsnKyQeONjPYq8m4rld7XVtjUBPd0qtcy Content-Type: multipart/mixed; boundary="oeHFM0qpQfkuo6aBheASaIKnW8bbYUcrc"; protected-headers="v1" From: Benedict Reuschling To: Bruce Evans , Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Message-ID: Subject: Re: svn commit: r347951 - stable/12/lib/libc/stdlib References: <201905180315.x4I3F8g2080553@repo.freebsd.org> <20190518082838.GW2748@kib.kiev.ua> <20190518210514.V913@besplex.bde.org> In-Reply-To: <20190518210514.V913@besplex.bde.org> --oeHFM0qpQfkuo6aBheASaIKnW8bbYUcrc Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. 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. Regards, Benedict Am 18.05.19 um 07:45 schrieb Bruce Evans: > On Sat, 18 May 2019, Konstantin Belousov wrote: >=20 >> 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: >>> =C2=A0 MFC r347617: >>> =C2=A0 Add small EXAMPLE section to bsearch.3. >>> >>> =C2=A0 Submitted by:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 fernape (via Phabricator) >>> =C2=A0 Reviewed by:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 bcr, jilles, dab >>> =C2=A0 Approved by:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 bcr (man pages), jilles (src) >>> =C2=A0 Differential Revision:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= https://reviews.freebsd.org/D19902 >>> >>> Modified: >>> =C2=A0 stable/12/lib/libc/stdlib/bsearch.3 >>> Directory Properties: >>> =C2=A0 stable/12/=C2=A0=C2=A0 (props changed) >>> >>> Modified: stable/12/lib/libc/stdlib/bsearch.3 >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D >>> >>> --- stable/12/lib/libc/stdlib/bsearch.3=C2=A0=C2=A0=C2=A0 Sat May 18 = 02:02:14 >>> 2019=C2=A0=C2=A0=C2=A0 (r347950) >>> +++ stable/12/lib/libc/stdlib/bsearch.3=C2=A0=C2=A0=C2=A0 Sat May 18 = 03:15:07 >>> 2019=C2=A0=C2=A0=C2=A0 (r347951) >>> @@ -32,7 +32,7 @@ >>> =C2=A0.\"=C2=A0=C2=A0=C2=A0=C2=A0 @(#)bsearch.3=C2=A0=C2=A0=C2=A0 8.3= (Berkeley) 4/19/94 >>> =C2=A0.\" $FreeBSD$ >>> =C2=A0.\" >>> -.Dd February 22, 2013 >>> +.Dd May 15, 2019 >>> =C2=A0.Dt BSEARCH 3 >>> =C2=A0.Os >>> =C2=A0.Sh NAME >>> @@ -83,6 +83,61 @@ The >>> =C2=A0function returns a pointer to a matching member of the array, o= r a null >>> =C2=A0pointer if no match is found. >>> =C2=A0If 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 { >>> +=C2=A0=C2=A0=C2=A0 char name[5]; >>> +=C2=A0=C2=A0=C2=A0 int age; >>> +}; >=20 > This example has a high density of style bugs.=C2=A0 kib pointed out so= me. > Some of the others are: >=20 > (1) Not sorting the struct members on alignment.=C2=A0 (style(9) says t= o sort > =C2=A0=C2=A0=C2=A0 on use, then size, but means alignment). > (2) Not indenting the struct member names. > (3) Not use a prefix for struct member names.=C2=A0 This is not importa= nt for > =C2=A0=C2=A0=C2=A0 small programs. >=20 >>> + >>> +int >>> +compare(const void *key, const void *array_member) >>> +{ >>> +=C2=A0=C2=A0=C2=A0 int age =3D (intptr_t) key; >>> +=C2=A0=C2=A0=C2=A0 struct person person =3D *(struct person *) array= _member; >> These two lines contain at least three style(9) bugs, and at least one= >> warning at higher warning level. >> >>> + >>> +=C2=A0=C2=A0=C2=A0 return (age - person.age); >>> +} >>> + >>> +int >>> +main() >> Why use K&R definition ? >> >>> +{ >>> +=C2=A0=C2=A0=C2=A0 struct person *friend; >>> + >>> +=C2=A0=C2=A0=C2=A0 /* Sorted array */ >>> +=C2=A0=C2=A0=C2=A0 struct person friends[6] =3D { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "paul", 22 }, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "anne", 25 }, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "fred", 25 }, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "mary", 27 }, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "mark", 35 }, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 { "bill", 50 } >>> +=C2=A0=C2=A0=C2=A0 }; >>> + >>> +=C2=A0=C2=A0=C2=A0 size_t array_size =3D sizeof(friends) / sizeof(st= ruct person); >> Since you used const elsewere, why did not you used it there ? >=20 > (4) The size expression is obfuscated by spelling the element size as > =C2=A0=C2=A0=C2=A0 sizeof(struct person) instead of sizeof(friends[0]).= > (5?) FreeBSD now spells this size expression as nitems(friends).=C2=A0 = I don't > =C2=A0=C2=A0=C2=A0 like the unportability of this, especially in an exa= mple. >=20 >> >>> + >>> +=C2=A0=C2=A0=C2=A0 friend =3D bsearch((void *)22, &friends, array_si= ze, sizeof(struct >>> person), compare); >> Taking address of an array is weird. >> Line is too long. >=20 > (6) C99 specifies that the search is for a member of the array that mat= ches > =C2=A0=C2=A0=C2=A0 the object pointed to by 'key'.=C2=A0 Here the key o= f (void *)22 is a > =C2=A0=C2=A0=C2=A0 not a pointer to an object.=C2=A0 It is a cookie whi= ch points to garbage. > =C2=A0=C2=A0=C2=A0 The cookie is unique enough to work in practice.=C2=A0= Perhaps you can > =C2=A0=C2=A0=C2=A0 prove it to always work if the option type intptr_t = is supported. > =C2=A0=C2=A0=C2=A0 But this is a bad example. > (4a) same obfuscation of the element size. >=20 >> >>> +=C2=A0=C2=A0=C2=A0 assert(strcmp(friend->name, "paul") =3D=3D 0); >>> +=C2=A0=C2=A0=C2=A0 printf("name: %s\enage: %d\en", friend->name, fri= end->age); >>> + >=20 > (7) Extra blank line.=C2=A0 This is not too bad, but is not done consis= tently. >=20 >>> +=C2=A0=C2=A0=C2=A0 friend =3D bsearch((void *)25, &friends, array_si= ze, sizeof(struct >>> person), compare); >=20 > (8) More too-long lines. > (4b) More sizeof(person)'s. >=20 >>> +=C2=A0=C2=A0=C2=A0 assert(strcmp(friend->name, "fred") =3D=3D 0 || s= trcmp(friend->name, >>> "anne") =3D=3D 0); >=20 > (7a) No extra blank line for "fred". >=20 >>> +=C2=A0=C2=A0=C2=A0 printf("name: %s\enage: %d\en", friend->name, fri= end->age); >>> + >>> +=C2=A0=C2=A0=C2=A0 friend =3D bsearch((void *)30, &friends, array_si= ze, sizeof(struct >>> person), compare); >>> +=C2=A0=C2=A0=C2=A0 assert(friend =3D=3D NULL); >>> +=C2=A0=C2=A0=C2=A0 printf("friend aged 30 not found\en"); >=20 > I didn't notice before that the key cookie was an encoding of the age. >=20 > The key is not unique (there are 2 25's).=C2=A0 This gives an example o= f low > quality data and the example has minimal error handling handling for th= is > without describing what it is doing (it asserts uniqueness for age 22 a= nd > it asserts one of the 2 possibilities for age 25.=C2=A0 It can only do = this > because it knows too much about the data). >=20 > 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 suppose= d > to be good. >=20 >>> + >>> +=C2=A0=C2=A0=C2=A0 return (EXIT_SUCCESS); >>> +} >>> +.Ed >>> =C2=A0.Sh SEE ALSO >>> =C2=A0.Xr db 3 , >>> =C2=A0.Xr lsearch 3 , >=20 > Bruce --oeHFM0qpQfkuo6aBheASaIKnW8bbYUcrc-- --fsnKyQeONjPYq8m4rld7XVtjUBPd0qtcy Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEwXonQDvNfP/33IoBVXQ7/QHhjTUFAlzf/OgACgkQVXQ7/QHh jTWulwf/Uyhskg3eKPZTWC7AmQ8KUm1K4wA3COyTChvmqgBgFtiXIOsQvxksq6rn 8lwQBY7zCI+xk6Bz0TSaCwrNRvttDsCbSWkoFLQNP/yh+ZupgM6Br5cGXNLdBN2U 64gkswvSJVwc//1VuOS87qC18msb6h5e1YNmsoh9E+08xPPu5QRx3TpR8ej0xQ8h ArAMB/BLG8/7zDbnjrwsTowRPmpZOUZ1RsSJ7P1Ps96KXun1nzYnVJoCPN+lAfLD UZgyKE6McWhCtEQniHGc0B0cpjhcILJoQ0wVtASZGsgMj12vk9ShccBcRXJj8zDK YzmhGsLDhq6BDUnObz3VMbgh9JKrJQ== =7Q1n -----END PGP SIGNATURE----- --fsnKyQeONjPYq8m4rld7XVtjUBPd0qtcy--