Re: bugs in dirname module - coreutils portion

[Jim Meyering]

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;
        }
    }