| Message ID | 1477393113-3845-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > * string/strtok.c (strtok): Return null is previous input is also s/is/if/ > diff --git a/string/strtok.c b/string/strtok.c > index 7a4574d..5c4b309 100644 > --- a/string/strtok.c > +++ b/string/strtok.c > @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) > { > char *token; > > - if (s == NULL) > - s = olds; > + if ((s == NULL) && ((s = olds) == NULL)) Please avoid assignment in an expression. And the parens are redundant. Andreas.
On 25/10/2016 09:31, Andreas Schwab wrote: > On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> * string/strtok.c (strtok): Return null is previous input is also > > s/is/if/ > >> diff --git a/string/strtok.c b/string/strtok.c >> index 7a4574d..5c4b309 100644 >> --- a/string/strtok.c >> +++ b/string/strtok.c >> @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) >> { >> char *token; >> >> - if (s == NULL) >> - s = olds; >> + if ((s == NULL) && ((s = olds) == NULL)) > > Please avoid assignment in an expression. And the parens are redundant. > > Andreas. > Right, with these fixes would it be acceptable?
On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 25/10/2016 09:31, Andreas Schwab wrote: >> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> >>> * string/strtok.c (strtok): Return null is previous input is also >> >> s/is/if/ >> >>> diff --git a/string/strtok.c b/string/strtok.c >>> index 7a4574d..5c4b309 100644 >>> --- a/string/strtok.c >>> +++ b/string/strtok.c >>> @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) >>> { >>> char *token; >>> >>> - if (s == NULL) >>> - s = olds; >>> + if ((s == NULL) && ((s = olds) == NULL)) >> >> Please avoid assignment in an expression. And the parens are redundant. >> >> Andreas. >> > > Right, with these fixes would it be acceptable? I don't see much point in supporting invalid use of strtok. Andreas.
On 25/10/2016 10:57, Andreas Schwab wrote: > On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> On 25/10/2016 09:31, Andreas Schwab wrote: >>> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>> >>>> * string/strtok.c (strtok): Return null is previous input is also >>> >>> s/is/if/ >>> >>>> diff --git a/string/strtok.c b/string/strtok.c >>>> index 7a4574d..5c4b309 100644 >>>> --- a/string/strtok.c >>>> +++ b/string/strtok.c >>>> @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) >>>> { >>>> char *token; >>>> >>>> - if (s == NULL) >>>> - s = olds; >>>> + if ((s == NULL) && ((s = olds) == NULL)) >>> >>> Please avoid assignment in an expression. And the parens are redundant. >>> >>> Andreas. >>> >> >> Right, with these fixes would it be acceptable? > > I don't see much point in supporting invalid use of strtok. > > Andreas. > My point is just to add portability and align with other current implementations.
On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 25/10/2016 10:57, Andreas Schwab wrote: >> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> >>> On 25/10/2016 09:31, Andreas Schwab wrote: >>>> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>>> >>>>> * string/strtok.c (strtok): Return null is previous input is also >>>> >>>> s/is/if/ >>>> >>>>> diff --git a/string/strtok.c b/string/strtok.c >>>>> index 7a4574d..5c4b309 100644 >>>>> --- a/string/strtok.c >>>>> +++ b/string/strtok.c >>>>> @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) >>>>> { >>>>> char *token; >>>>> >>>>> - if (s == NULL) >>>>> - s = olds; >>>>> + if ((s == NULL) && ((s = olds) == NULL)) >>>> >>>> Please avoid assignment in an expression. And the parens are redundant. >>>> >>>> Andreas. >>>> >>> >>> Right, with these fixes would it be acceptable? >> >> I don't see much point in supporting invalid use of strtok. >> >> Andreas. >> > > My point is just to add portability and align with other current > implementations. Has it ever be a problem in the past? Andreas.
On 25/10/2016 11:19, Andreas Schwab wrote: > On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> On 25/10/2016 10:57, Andreas Schwab wrote: >>> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>> >>>> On 25/10/2016 09:31, Andreas Schwab wrote: >>>>> On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>>>> >>>>>> * string/strtok.c (strtok): Return null is previous input is also >>>>> >>>>> s/is/if/ >>>>> >>>>>> diff --git a/string/strtok.c b/string/strtok.c >>>>>> index 7a4574d..5c4b309 100644 >>>>>> --- a/string/strtok.c >>>>>> +++ b/string/strtok.c >>>>>> @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) >>>>>> { >>>>>> char *token; >>>>>> >>>>>> - if (s == NULL) >>>>>> - s = olds; >>>>>> + if ((s == NULL) && ((s = olds) == NULL)) >>>>> >>>>> Please avoid assignment in an expression. And the parens are redundant. >>>>> >>>>> Andreas. >>>>> >>>> >>>> Right, with these fixes would it be acceptable? >>> >>> I don't see much point in supporting invalid use of strtok. >>> >>> Andreas. >>> >> >> My point is just to add portability and align with other current >> implementations. > > Has it ever be a problem in the past? > > Andreas. > None I am aware of, but regardless it is a effort to close down old glibc bugs and keep the backlog under control.
On 10/25/2016 12:58 PM, Adhemerval Zanella wrote:
> The original bug report comment #1
I don't see a reference to that bug anywhere, so it's not clear (to me
at least) what the report was about. (But I can guess …)
Florian
On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > None I am aware of, but regardless it is a effort to close down old > glibc bugs and keep the backlog under control. Which bug? Andreas.
On 25/10/2016 11:45, Andreas Schwab wrote: > On Okt 25 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> None I am aware of, but regardless it is a effort to close down old >> glibc bugs and keep the backlog under control. > > Which bug? > > Andreas. > Oops, my bad. BZ#16640.
On Tue, 25 Oct 2016, Adhemerval Zanella wrote: > None I am aware of, but regardless it is a effort to close down old > glibc bugs and keep the backlog under control. Well, if a bug report is invalid then closing it as INVALID is appropriate. Or if there is an idea in the bug report that might or might not be a good idea but isn't appropriate for Bugzilla, closing as INVALID and putting a note on <https://sourceware.org/glibc/wiki/Development_Todo/Master> of the idea to consider (with a link to the previous discussion in the bug) is appropriate.
On 25/10/2016 11:51, Joseph Myers wrote: > On Tue, 25 Oct 2016, Adhemerval Zanella wrote: > >> None I am aware of, but regardless it is a effort to close down old >> glibc bugs and keep the backlog under control. > > Well, if a bug report is invalid then closing it as INVALID is > appropriate. Or if there is an idea in the bug report that might or might > not be a good idea but isn't appropriate for Bugzilla, closing as INVALID > and putting a note on > <https://sourceware.org/glibc/wiki/Development_Todo/Master> of the idea to > consider (with a link to the previous discussion in the bug) is > appropriate. > Right, but the bug report is about the inconsistent behaviour about for x86_64 (and powerpc as well) and default one. Bug report comments from Carlos pointed that it should be fixed in x86_64/powerpc implementation, while I argued that it would better to follow what other libc are aiming for since this specific case that does trigger any particular issue. That's why I think it is not invalid. If the consensus is indeed to fix the x86_64/powerpc I will work towards, although I still prefer aim for portability.
diff --git a/string/strtok.c b/string/strtok.c index 7a4574d..5c4b309 100644 --- a/string/strtok.c +++ b/string/strtok.c @@ -40,8 +40,8 @@ STRTOK (char *s, const char *delim) { char *token; - if (s == NULL) - s = olds; + if ((s == NULL) && ((s = olds) == NULL)) + return NULL; /* Scan leading delimiters. */ s += strspn (s, delim); diff --git a/string/tst-strtok.c b/string/tst-strtok.c index 6fbef9f..d9180a4 100644 --- a/string/tst-strtok.c +++ b/string/tst-strtok.c @@ -2,25 +2,26 @@ #include <stdio.h> #include <string.h> +static int do_test (void); + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" + static int do_test (void) { char buf[1] = { 0 }; int result = 0; + if (strtok (NULL, " ") != NULL) + FAIL_RET ("first strtok call did not return NULL"); + if (strtok (NULL, " ") != NULL) + FAIL_RET ("second strtok call did not return NULL"); + if (strtok (buf, " ") != NULL) - { - puts ("first strtok call did not return NULL"); - result = 1; - } + FAIL_RET ("third strtok call did not return NULL"); else if (strtok (NULL, " ") != NULL) - { - puts ("second strtok call did not return NULL"); - result = 1; - } + FAIL_RET ("forth strtok call did not return NULL"); return result; } - -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c"