Re: usr.bin/man: use getlist(char *) instead of hand-rolled equivalent in config(char *)
Thanks for the feedback, Ingo! Sorry for the confusion. I'll spend some more
time with queue(3). :)
> On Jul 17, 2014, at 1:21, Ingo Schwarze wrote:
>
> Hi,
>
> Kent R. Spillner wrote on Wed, Jun 04, 2014 at 10:01:12AM -0500:
>
>> config(char *) contains a hand-rolled version of getlist(char *).
>
> Indeed.
>
>> The only difference is that the hand-rolled version includes a NULL
>> check before the strcmp.
>
> You misread the code. There is no NULL check for the string p.
> The NULL check for tp is just the usual NULL check contained
> in the FOREACH macro to stop at the end of the list.
>
>> Replace this with a call to getlist(char *) instead,
>
> That is correct and adds clarity IMHO.
>
>> and move the NULL check there to protect other callers as well.
>
> You are moving no NULL check but adding a completely new one.
> That new NULL check seems pointless.
>
>> I think we can probably drop the NULL check altogether because we
>> already check p is not NULL as the loop condition in the for-loop.
>
> Correct.
>
>> However, since the original code had an explicit check I decided
>> to preserve it.
>
> As explained above, it had not.
>
>> I'm ok dropping the second hunk of this diff
>> if others agree it's unnecessary.
>
> Any OKs to commit just the first chunk?
> Ingo
>
>
> Index: usr.bin/man/config.c
> ===
> RCS file: /work/cvsroot/src/usr.bin/man/config.c,v
> retrieving revision 1.9
> diff -p -u -r1.9 config.c
> --- usr.bin/man/config.c27 Oct 2009 23:59:40 -1.9
> +++ usr.bin/man/config.c4 Jun 2014 14:51:10 -
> @@ -92,8 +92,7 @@ config(char *fname)
>continue;
>*t = '\0';
>
> -for (tp = TAILQ_FIRST(&head);/* Find any matching tag. */
> -tp != NULL && strcmp(p, tp->s); tp = TAILQ_NEXT(tp, q));
> +tp = getlist(p);/* Find any matching tag. */
>
>if (tp == NULL)/* Create a new tag. */
>tp = addlist(p);
>
>
> This is not useful:
>
>> @@ -147,6 +146,9 @@ TAG *
>> getlist(char *name)
>> {
>>TAG *tp;
>> +
>> +if (name == NULL)
>> +return (NULL);
>>
>>TAILQ_FOREACH(tp, &head, q)
>>if (!strcmp(name, tp->s))
>
Re: usr.bin/man: use getlist(char *) instead of hand-rolled equivalent in config(char *)
Hi,
Kent R. Spillner wrote on Wed, Jun 04, 2014 at 10:01:12AM -0500:
> config(char *) contains a hand-rolled version of getlist(char *).
Indeed.
> The only difference is that the hand-rolled version includes a NULL
> check before the strcmp.
You misread the code. There is no NULL check for the string p.
The NULL check for tp is just the usual NULL check contained
in the FOREACH macro to stop at the end of the list.
> Replace this with a call to getlist(char *) instead,
That is correct and adds clarity IMHO.
> and move the NULL check there to protect other callers as well.
You are moving no NULL check but adding a completely new one.
That new NULL check seems pointless.
> I think we can probably drop the NULL check altogether because we
> already check p is not NULL as the loop condition in the for-loop.
Correct.
> However, since the original code had an explicit check I decided
> to preserve it.
As explained above, it had not.
> I'm ok dropping the second hunk of this diff
> if others agree it's unnecessary.
Any OKs to commit just the first chunk?
Ingo
Index: usr.bin/man/config.c
===
RCS file: /work/cvsroot/src/usr.bin/man/config.c,v
retrieving revision 1.9
diff -p -u -r1.9 config.c
--- usr.bin/man/config.c27 Oct 2009 23:59:40 - 1.9
+++ usr.bin/man/config.c4 Jun 2014 14:51:10 -
@@ -92,8 +92,7 @@ config(char *fname)
continue;
*t = '\0';
- for (tp = TAILQ_FIRST(&head); /* Find any matching tag. */
- tp != NULL && strcmp(p, tp->s); tp = TAILQ_NEXT(tp, q));
+ tp = getlist(p);/* Find any matching tag. */
if (tp == NULL) /* Create a new tag. */
tp = addlist(p);
This is not useful:
> @@ -147,6 +146,9 @@ TAG *
> getlist(char *name)
> {
> TAG *tp;
> +
> + if (name == NULL)
> + return (NULL);
>
> TAILQ_FOREACH(tp, &head, q)
> if (!strcmp(name, tp->s))
Re: usr.bin/man: use getlist(char *) instead of hand-rolled equivalent in config(char *)
*Bump*
> On Jul 10, 2014, at 12:34, "Kent R. Spillner" wrote:
>
> Ping.
>
>> On Wed, Jun 04, 2014 at 10:01:12AM -0500, Kent R. Spillner wrote:
>> config(char *) contains a hand-rolled version of getlist(char *). The only
>> difference
>> is that the hand-rolled version includes a NULL check before the strcmp.
>> Replace this
>> with a call to getlist(char *) instead, and move the NULL check there to
>> protect other
>> callers as well.
>>
>> I think we can probably drop the NULL check altogether because we already
>> check p is not
>> NULL as the loop condition in the for-loop. However, since the original
>> code had an
>> explicit check I decided to preserve it. I'm ok dropping the second hunk of
>> this diff
>> if others agree it's unnecessary.
>>
>>
>> Index: usr.bin/man/config.c
>> ===
>> RCS file: /work/cvsroot/src/usr.bin/man/config.c,v
>> retrieving revision 1.9
>> diff -p -u -r1.9 config.c
>> --- usr.bin/man/config.c27 Oct 2009 23:59:40 -1.9
>> +++ usr.bin/man/config.c4 Jun 2014 14:51:10 -
>> @@ -92,8 +92,7 @@ config(char *fname)
>>continue;
>>*t = '\0';
>>
>> -for (tp = TAILQ_FIRST(&head);/* Find any matching tag. */
>> -tp != NULL && strcmp(p, tp->s); tp = TAILQ_NEXT(tp, q));
>> +tp = getlist(p);/* Find any matching tag. */
>>
>>if (tp == NULL)/* Create a new tag. */
>>tp = addlist(p);
>> @@ -147,6 +146,9 @@ TAG *
>> getlist(char *name)
>> {
>>TAG *tp;
>> +
>> +if (name == NULL)
>> +return (NULL);
>>
>>TAILQ_FOREACH(tp, &head, q)
>>if (!strcmp(name, tp->s))
>
Re: usr.bin/man: use getlist(char *) instead of hand-rolled equivalent in config(char *)
Ping.
On Wed, Jun 04, 2014 at 10:01:12AM -0500, Kent R. Spillner wrote:
> config(char *) contains a hand-rolled version of getlist(char *). The only
> difference
> is that the hand-rolled version includes a NULL check before the strcmp.
> Replace this
> with a call to getlist(char *) instead, and move the NULL check there to
> protect other
> callers as well.
>
> I think we can probably drop the NULL check altogether because we already
> check p is not
> NULL as the loop condition in the for-loop. However, since the original code
> had an
> explicit check I decided to preserve it. I'm ok dropping the second hunk of
> this diff
> if others agree it's unnecessary.
>
>
> Index: usr.bin/man/config.c
> ===
> RCS file: /work/cvsroot/src/usr.bin/man/config.c,v
> retrieving revision 1.9
> diff -p -u -r1.9 config.c
> --- usr.bin/man/config.c 27 Oct 2009 23:59:40 - 1.9
> +++ usr.bin/man/config.c 4 Jun 2014 14:51:10 -
> @@ -92,8 +92,7 @@ config(char *fname)
> continue;
> *t = '\0';
>
> - for (tp = TAILQ_FIRST(&head); /* Find any matching tag. */
> - tp != NULL && strcmp(p, tp->s); tp = TAILQ_NEXT(tp, q));
> + tp = getlist(p);/* Find any matching tag. */
>
> if (tp == NULL) /* Create a new tag. */
> tp = addlist(p);
> @@ -147,6 +146,9 @@ TAG *
> getlist(char *name)
> {
> TAG *tp;
> +
> + if (name == NULL)
> + return (NULL);
>
> TAILQ_FOREACH(tp, &head, q)
> if (!strcmp(name, tp->s))
>
