[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] add new module filevercmp

From: Jim Meyering
Subject: Re: [PATCH] add new module filevercmp
Date: Sat, 27 Sep 2008 10:47:51 +0200

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:

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.  */
 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?

-/* 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,

-   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:



reply via email to

[Prev in Thread] Current Thread [Next in Thread]