From owner-svn-src-all@freebsd.org Sat May 18 11:45:36 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 CB3E115AFE58; Sat, 18 May 2019 11:45:36 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 1FABC7627C; Sat, 18 May 2019 11:45:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 5DE593DC531; Sat, 18 May 2019 21:45:20 +1000 (AEST) Date: Sat, 18 May 2019 21:45:19 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Benedict Reuschling , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: Re: svn commit: r347951 - stable/12/lib/libc/stdlib In-Reply-To: <20190518082838.GW2748@kib.kiev.ua> Message-ID: <20190518210514.V913@besplex.bde.org> References: <201905180315.x4I3F8g2080553@repo.freebsd.org> <20190518082838.GW2748@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=sVDMQsUGGj2q3MpuiyMA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: 1FABC7627C X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-7.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; NEURAL_HAM_SHORT(-1.00)[-0.997,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] 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: Sat, 18 May 2019 11:45:37 -0000 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