Re: [PATCH] add new module filevercmp

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:
> thank you for review. I've made the changes you requested, new patch attached.

Hi Kamil,

I see a warning due to unnecessary test module dependent:

  $ ./gnulib-tool --dir /tmp/pm --create-testdir --with-tests --test filevercmp
  warning: module filevercmp-tests has duplicated dependencies: filevercmp
  Module list with included dependencies:
    extensions
  ...

I've included the fix for that along with some suggested comment changes
below.  However, there's a minor discrepancy between comments and code that
you'll have to address:

The IS* macros imply that ISALPHA and ISALNUM are intended to
be locale-sensitive, whereas the match_suffix comment says that
function is not.  For reference, here they are:

    #define ISALNUM(c) isalnum ((unsigned char) (c))
    #define ISDIGIT(c) isdigit ((unsigned char) (c))
    #define ISALPHA(c) isalpha ((unsigned char) (c))

    /*
       match file suffix defined as RE (\.[A-Za-z][A-Za-z0-9]*)*$

       Scan string pointed by *str and return pointer to suffix begin or NULL if
       not found. Pointer *str points to ending zero of scanned string after
       return. */
    static const char *
    match_suffix (const char **str)

To have any hope of sanity/reproducibility in different locales,
this function must return the same result, regardless of the current
locale settings.  So...

You should use the c_isalnum and c_isalpha functions in place of the
macros.  They come from the c-ctype module and the "c-ctype.h" header.
And I suggest using this macro in place of ISDIGIT:

    #define ISDIGIT(c) ((unsigned int) (c) - '0' <= 9)


diff --git a/lib/filevercmp.c b/lib/filevercmp.c
index bc641e5..3ec34ef 100644
--- a/lib/filevercmp.c
+++ b/lib/filevercmp.c
@@ -118,9 +118,8 @@ verrevcmp (const char *s1, size_t s1_len, const char *s2, 
size_t s2_len)
   return 0;
 }

-/* function comparing version strings
-
-   see filevercmp.h for function description */
+/* Compare version strings S1 and S2.
+   See filevercmp.h for function description.  */
 int
 filevercmp (const char *s1, const char *s2)
 {
@@ -130,8 +129,9 @@ filevercmp (const char *s1, const char *s2)
   size_t s1_len, s2_len;
   int result;

-  /* easy comparison to see if versions are identical */
-  if (!strcmp (s1, s2))
+  /* easy comparison to see if strings are identical */
+  int simple_cmp = strcmp (s1, s2);
+  if (simple_cmp == 0)
     return 0;

   /* "cut" file suffixes */
@@ -149,8 +149,5 @@ filevercmp (const char *s1, const char *s2)
     }

   result = verrevcmp (s1, s1_len, s2, s2_len);
-  return (result == 0)
-    /* return 0 if (and only if) strings S1 and S2 are identical */
-    ? strcmp (s1, s2) : result;
+  return result == 0 ? simple_cmp : result;
 }
-
diff --git a/lib/filevercmp.h b/lib/filevercmp.h
index 6447fea..2f40cdd 100644
--- a/lib/filevercmp.h
+++ b/lib/filevercmp.h
@@ -17,7 +17,7 @@ TODO: copyright?
 #ifndef FILEVERCMP_H
 #define FILEVERCMP_H

-/* function comparing version strings
+/* Compare version strings:

    This function compares strings S1 and S2:
    1) By PREFIX in the same way as strcmp.
@@ -34,7 +34,7 @@ TODO: copyright?
    are strings then VER1 < VER2 implies filevercmp (PREFIX VER1 SUFFIX,
    PREFIX VER2 SUFFIX) < 0.

-   This function is intended to be replacement for strverscmp. */
+   This function is intended to be a replacement for strverscmp. */
 int filevercmp (const char *s1, const char *s2);

 #endif /* FILEVERCMP_H */
diff --git a/modules/filevercmp-tests b/modules/filevercmp-tests
index 02554fe..165ecfe 100644
--- a/modules/filevercmp-tests
+++ b/modules/filevercmp-tests
@@ -2,7 +2,6 @@ Files:
 tests/test-filevercmp.c

 Depends-on:
-filevercmp

 configure.ac: