Re: Patch for x86_32: Fix bug 14195

[Dmitrieva Liubov <liubov dot dmitrieva at gmail dot com>]

Thanks for the review.

> Missing copyright year update and merge.
Added.

> Please add a comment saying "/* Regression test for BZ #14195.  */
Done.

> The code you are adding should be in a function labelled after
> the BZ# that you are fixing e.g. bz14195();.
Fixed.

I've reattached the patch and Change Log is the same.

ChangeLog:

2012-08-15  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

--
Liubov Dmitrieva

2012/8/14 Carlos O'Donell <carlos_odonell@mentor.com>:
> 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

Attachment: strncasecmp_fix.patch
Description: Binary data