[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bugs in dirname module - coreutils portion
From: |
Jim Meyering |
Subject: |
Re: bugs in dirname module - coreutils portion |
Date: |
Wed, 23 Nov 2005 20:57:56 +0100 |
Eric Blake <address@hidden> wrote:
> This is my patch to coreutils to match the dirname module updates thread
> on gnulib: http://lists.gnu.org/archive/html/bug-gnulib/2005-11/msg00071.html
>
> I've also fixed the two nits that Paul had with the latest version of my
> gnulib patch, mentioned in
> http://lists.gnu.org/archive/html/bug-gnulib/2005-11/msg00076.html.
>
> ChangeLog:
> 2005-11-23 Eric Blake <address@hidden>
>
> * tests/dirname/basic: New file.
> * tests/dirname/Makefile.am: New file.
> * tests/basename/basic: Add some tests.
> * tests/Makefile.am (SUBDIRS): Add dirname tests.
> * configure.ac (AC_CONFIG_FILES): Likewise.
Thanks for doing all of that.
I haven't looked through it much at all yet, but for starters,
would you please add the new tests in tests/misc/,
say as tests/misc/dirname and tests/misc/basename,
instead of as separate directories?
That's what we did for the new sha*sum programs.
Originally, the testing framework required one directory per
Test.pm file, but that is obviously no longer a limitation, and
I think there are already too many directories under tests/.
Also, a style issue:
+ while (ISSLASH (*base))
+ base++;
+ for (p = base; *p; p++)
+ if (ISSLASH (*p))
+ saw_slash = true;
+ else if (saw_slash)
+ {
+ base = p;
+ saw_slash = false;
+ }
Please do not omit braces unless the block they would
enclose consists of a single line -- adding a blank
line between the loops is nice, too.
I find the following to be a little more readable/maintainable.
while (ISSLASH (*base))
base++;
for (p = base; *p; p++)
{
if (ISSLASH (*p))
saw_slash = true;
else if (saw_slash)
{
base = p;
saw_slash = false;
}
}