[illumos-Developer] tr webrev

[Garrett D'Amore]

Thanks for your work on this John.

I'm going to investigate this further before I approve the review, but
only because I want to make sure that casting is the "right" solution
here, and its not a sign of something more fundamentally wrong in the
return value or type.

	- Garrett

On Mon, 2010-09-06 at 22:12 -0500, John Grafton wrote:
> I found a bug in the FreeBSD tr implementation that's causing it to
> fail most tests.  I submitted a new webrev a few minutes ago which
> changes one line in cset.c.
> 
> 
> The entire problem with the FreeBSD tr seems to revolve around an
> incorrect return of cset_rangecmp (line 158 of cset.c).  The incorrect
> return corrupts the cache for all values below the lowest value in
> your search range.  The splay tree functions work great, but since the
> cache building process was broken, all functions that relied upon the
> cache (most of them) failed to correctly identify the proper
> characters.
> 
> 
> The fix was simple, add a bool typecast to the return.  Change line
> 158 of cset.c from:
> return ((cs->cs_invert ^ cset_rangecmp(cs->cs_root, ch)) == 0);
> 
> 
> to:
> return ((cs->cs_invert ^ (bool) cset_rangecmp(cs->cs_root, ch)) == 0);
> 
> 
> http://cr.illumos.org/view/b8z5ojjd/
> 
> 
> Thanks!
> John
> 
> On Mon, Sep 6, 2010 at 6:48 PM, Joerg Schilling
> <Joerg.Schilling at fokus.fraunhofer.de> wrote:
>         Paul Armstrong <illumos at otoh.org> wrote:
>         
>         > At 2010-09-06T13:29+0200, Joerg Schilling wrote:
>         > > "Garrett D'Amore" <garrett at nexenta.com> wrote:
>         > > Unless we have an automated test for tr that would allow
>         to test
>         > > everything (which is not possible), we could create a
>         "testing" source
>         > > gate and encourage people to do testing for a longer
>         period of time
>         > > until we could move the tested bits into the stable gate.
>         >
>         > So this seems like a good time to bring up the question of
>         where we put
>         > tests.
>         
>         
>         I am usually a fan of separated tests. but I usually only
>         change few things
>         before I test and publish.
>         
>         We are in a different situation that had only once before,
>         when I decided that
>         GNU getlongopt is too buggy for mkisofs and replaced it by my
>         getargs(). This
>         was a really big change (a few thousand lines) and the changes
>         had to replace
>         much code that was related to verification of option
>         combinations. I was not
>         able to do a systematic test here and it took me half a year
>         to find and fix
>         the three small bugs I introduced by the change.
>         
>         We now have a similar situation and it may be that there is
>         only the option to
>         close euyes and try to live with the problems (that however
>         need to be fixed
>         soon after they will be detected).
>         
>         Our problem is caused by the need to replace the i18n code
>         that is expected to
>         cause more problems.
>         
>         
>         > We can either have a separate gate or we can create "test"
>         directories
>         > within the directory of the module to be tested (e.g.
>         > illumos/usr/src/cmd/zpool/test).
>         
>         
>         You can go this path if you change few things and know a test
>         strategy.
>         
>         In our case it helps that we now know that there have been no
>         real tests before.
>         I could write some tests based on the POSIX documentation.
>         
>         In any case, a source based code review is not sufficient, we
>         need to also test
>         the binary. As a typical Solaris boot does not really call
>         tr(1), we should
>         try to do systematic testing. Since today, this is easier as
>         there is a SchillIX
>         based on Illumos, so there is an easy to reproduce
>         environment.
>         
>         BTW: has someone an idea that allows to create automated test
>         case compilations
>         that are based on:
>         
>         -       commandline
>         -       input
>         -       expected stdout
>         -       extected stderr
>         
>         Jörg
>         
>         --
>          EMail:joerg at schily.isdn.cs.tu-berlin.de (home) Jörg Schilling
>         D-13353 Berlin
>               js at cs.tu-berlin.de                (uni)
>               joerg.schilling at fokus.fraunhofer.de (work) Blog:
>         http://schily.blogspot.com/
>          URL:  http://cdrecord.berlios.de/private/
>         ftp://ftp.berlios.de/pub/schily
>         
>         
>         
>         _______________________________________________
>         Developer mailing list
>         Developer at lists.illumos.org
>         http://lists.illumos.org/m/listinfo/developer
>         
> 
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer