This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]