Patch for x86_32: Fix bug 14195
Andreas Jaeger
aj@suse.com
Wed Aug 15 19:04:00 GMT 2012
On 08/15/2012 11:33 AM, Dmitrieva Liubov wrote:
> 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
The changelog should be IMO:
* string/test-strncasecmp.c (check1): Renamed to...
(bz12205): ...this.
(bz14195): Add new testcase.
(test_main): Call new testcase, adapt for renamed function.
> --
> 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.
Merge means: replace 2008, 2010, 2012 with
2008-2012
I'm committing the change now for you since AFAIK you don't have git access,
Thanks,
Andreas
>>
>>> @@ -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
--
Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126
More information about the Libc-alpha
mailing list