This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Patch for x86_32: Fix bug 14195
On 8/14/2012 7:15 AM, Dmitrieva Liubov wrote:
> Ok, fixed version is attached.
Thanks for working through the changes.
It would be useful if you provide a summary of the changes since the
last version, that way the reviewers can see that you either accepted
or refuted all of the comments.
> ChangeLog:
>
> 2012-08-14 Liubov Dmitrieva <liubov.dmitrieva@gmail.com>
>
> [BZ #14195]
> * sysdeps/i386/i686/multiarch/strcmp-sssse3.S: Fix
> segmentation fault for a case of two empty input strings.
> * string/test-strncasecnp.c: Update.
> Add a test-case for two empty input strings and N > 0
Please pay more careful attention to the reviewer comments. There
are some things which you did not fix and they are important.
> strncasecmp_fix.patch
>
>
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6c17530..1b90e06 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
Missing copyright year update and merge.
> @@ -270,6 +270,14 @@ check1 (void)
> check_result (impl, s1, s2, n, exp_result);
> }
>
Please add a comment saying "/* Regression test for BZ #14195. */
> +static void
> +bz12205 (void)
This is wrong. Please review what I said in my original email.
BZ #12205 is the BZ # for the regression test that was added
as check1();.
The code you are adding should be in a function labelled after
the BZ# that you are fixing e.g. bz14195();.
> +{
> + const char *empty_string = "";
> + FOR_EACH_IMPL (impl, 0)
> + check_result (impl, empty_string, "", 5, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -278,6 +286,7 @@ test_main (void)
> test_init ();
>
> check1 ();
> + bz12205 ();
>
> printf ("%23s", "");
> FOR_EACH_IMPL (impl, 0)
> diff --git a/sysdeps/i386/i686/multiarch/strcmp-ssse3.S b/sysdeps/i386/i686/multiarch/strcmp-ssse3.S
> old mode 100644
> new mode 100755
> index 5e6321e..9735ad0
> --- a/sysdeps/i386/i686/multiarch/strcmp-ssse3.S
> +++ b/sysdeps/i386/i686/multiarch/strcmp-ssse3.S
> @@ -2445,7 +2445,7 @@ L(less16bytes_sncmp):
> # endif
> jne L(neq_sncmp)
> test %cl, %cl
> - je L(eq)
> + je L(eq_sncmp)
>
> cmp $1, REM
> je L(eq_sncmp)
Please fix the copyright year, function name, comment, and repost.
You're doing a good job, but please pay more careful attention to the reviewer comments.
Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026