Re: Patch for x86_32: Fix bug 14195

[Andreas Jaeger <aj at suse dot com>]

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