Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On 5/20/19 10:34 AM, Rodney W. Grimes wrote: >> On Sun, May 19, 2019 at 9:56 AM Yoshihiro Ota wrote: >> >>> I wonder if we can use a tool to confirm coding style like >>> clang-format or something else. >>> >> >> I don't know... it might be hard to do that inside a man page... > > Given the current tooling I concur, so we need .include > for mandoc, and teach mandoc how to format example code which > is mostly knows how to do, or atleast there was lots of groff > code to do that. I would prefer this approach if at all possible. -- John Baldwin ___ 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-unsubscr...@freebsd.org"
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
> On Sun, May 19, 2019 at 9:56 AM Yoshihiro Ota wrote: > > > I wonder if we can use a tool to confirm coding style like > > clang-format or something else. > > > > I don't know... it might be hard to do that inside a man page... Given the current tooling I concur, so we need .include for mandoc, and teach mandoc how to format example code which is mostly knows how to do, or atleast there was lots of groff code to do that. > Warner -- Rod Grimes rgri...@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-unsubscr...@freebsd.org"
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On Sun, 19 May 2019, Warner Losh wrote: On Sun, May 19, 2019 at 9:56 AM Yoshihiro Ota wrote: I wonder if we can use a tool to confirm coding style like clang-format or something else. I don't know... it might be hard to do that inside a man page... Examples in man pages need special treatment since man adds a 5 column left margin and then must mangle tabs to preserve 8-column indentation. The examples must at least be formatted for width 74 instead of 79, but 74 is better anyway. I had some success turning SYNOPSIS sections into C code for checking that the headers are complete and the documented prototypes agree with the headers. Large examples would probably have to be extracted manually since they are less structured than SYNOPSIS sections. Bruce ___ 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-unsubscr...@freebsd.org"
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On Sun, May 19, 2019 at 9:56 AM Yoshihiro Ota wrote: > I wonder if we can use a tool to confirm coding style like > clang-format or something else. > I don't know... it might be hard to do that inside a man page... Warner ___ 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-unsubscr...@freebsd.org"
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
Am 19.05.19 um 11:54 schrieb Yoshihiro Ota: > 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... > We were discussing that a little bit at BSDCan. clang-format has a few edge cases (or our style(9) needs to change (many potential bikesheds there). There was also talk about using annotations to give the tool some hints about which parts it should ignore that are different in our style. Ed Maste is the better person to talk to about it than me (and on separate different mailing list perhaps). Back the original issue: I reopened the review and have asked the submitter to incorporate the feedback from here. Regards to all Benedict > > 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 +, 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
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
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 +, 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 {
> > >>>
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
> 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 +, 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,
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On Sat, May 18, 2019 at 08:39:03AM -0400, Benedict Reuschling 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. > > 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. > Try to improve it. I think there is some value in having a good and non-buggy example for the function(s), but we are not there yet. You have a lot of comments with direct actions. ___ 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-unsubscr...@freebsd.org"
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
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:
>
>> On Sat, May 18, 2019 at 03:15:08AM +, 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 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, , 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, , array_size, sizeof(struct
>>> person), compare);
>
> (8) More too-long lines.
> (4b) More sizeof(person)'s.
>
>>> +
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On Sat, 18 May 2019, Konstantin Belousov wrote:
On Sat, May 18, 2019 at 03:15:08AM +, 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 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, , 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, , 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, , 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
___
svn-src-all@freebsd.org mailing list
Re: svn commit: r347951 - stable/12/lib/libc/stdlib
On Sat, May 18, 2019 at 03:15:08AM +, 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.38.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;
> +};
> +
> +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 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 ?
> +
> + friend = bsearch((void *)22, , array_size, sizeof(struct
> person), compare);
Taking address of an array is weird.
Line is too long.
> + assert(strcmp(friend->name, "paul") == 0);
> + printf("name: %s\enage: %d\en", friend->name, friend->age);
> +
> + friend = bsearch((void *)25, , array_size, sizeof(struct
> person), compare);
> + assert(strcmp(friend->name, "fred") == 0 || strcmp(friend->name,
> "anne") == 0);
> + printf("name: %s\enage: %d\en", friend->name, friend->age);
> +
> + friend = bsearch((void *)30, , array_size, sizeof(struct
> person), compare);
> + assert(friend == NULL);
> + printf("friend aged 30 not found\en");
> +
> + return (EXIT_SUCCESS);
> +}
> +.Ed
> .Sh SEE ALSO
> .Xr db 3 ,
> .Xr lsearch 3 ,
___
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-unsubscr...@freebsd.org"
svn commit: r347951 - stable/12/lib/libc/stdlib
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;
+};
+
+int
+compare(const void *key, const void *array_member)
+{
+ int age = (intptr_t) key;
+ struct person person = *(struct person *) array_member;
+
+ return (age - person.age);
+}
+
+int
+main()
+{
+ 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);
+
+ friend = bsearch((void *)22, , array_size, sizeof(struct
person), compare);
+ assert(strcmp(friend->name, "paul") == 0);
+ printf("name: %s\enage: %d\en", friend->name, friend->age);
+
+ friend = bsearch((void *)25, , array_size, sizeof(struct
person), compare);
+ assert(strcmp(friend->name, "fred") == 0 || strcmp(friend->name,
"anne") == 0);
+ printf("name: %s\enage: %d\en", friend->name, friend->age);
+
+ friend = bsearch((void *)30, , array_size, sizeof(struct
person), compare);
+ assert(friend == NULL);
+ printf("friend aged 30 not found\en");
+
+ return (EXIT_SUCCESS);
+}
+.Ed
.Sh SEE ALSO
.Xr db 3 ,
.Xr lsearch 3 ,
___
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-unsubscr...@freebsd.org"
