Re: [PATCH] add new module filevercmp

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:
> as it was mentioned in the thread at
> http://lists.gnu.org/archive/html/bug-gnulib/2008-09/msg00198.html I propose
> new gnulib module filevercmp - in the attachment.
...

Thanks for all that work!
A few comments in-line:

> From 842bccfc33b46570b73956e39be8c0d9c064453b Mon Sep 17 00:00:00 2001
> From: Kamil Dudka <address@hidden>
> Date: Wed, 24 Sep 2008 16:01:57 +0200
> Subject: [PATCH] add new module filevercmp
>
> * lib/filevercmp.h: New function FILEVERCMP comparing version strings
> (and file names with version).
> * lib/filevercmp.c: Implementation of FILEVERCMP function.
> * m4/filevercmp.m4: Link filevercmp module to library.
> * modules/filevercmp: Module metadata.
> * tests/test-filevercmp.c: Unit test for new module.
> * modules/filevercmp-tests: Unit test metadata.
> * MODULES.html.sh: Add FILEVERCMP module.
> * NEWS: Mention the change.
> ---
>  MODULES.html.sh          |    1 +
>  NEWS                     |    8 +++
>  lib/filevercmp.c         |  157 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  lib/filevercmp.h         |   29 +++++++++
>  m4/filevercmp.m4         |    7 ++
>  modules/filevercmp       |   24 +++++++
>  modules/filevercmp-tests |   11 +++
>  tests/test-filevercmp.c  |  101 +++++++++++++++++++++++++++++
>  8 files changed, 338 insertions(+), 0 deletions(-)
>  create mode 100644 lib/filevercmp.c
>  create mode 100644 lib/filevercmp.h
>  create mode 100644 m4/filevercmp.m4
>  create mode 100644 modules/filevercmp
>  create mode 100644 modules/filevercmp-tests
>  create mode 100644 tests/test-filevercmp.c
>
> diff --git a/MODULES.html.sh b/MODULES.html.sh
> index afaf8ba..3ab5a90 100755
> --- a/MODULES.html.sh
> +++ b/MODULES.html.sh
> @@ -1856,6 +1856,7 @@ func_all_modules ()
>    func_module readtokens
>    func_module readtokens0
>    func_module strverscmp
> +  func_module filevercmp
>    func_end_table
>
>    element="Support for systems lacking ISO C 99"
> diff --git a/NEWS b/NEWS
> index 16917cb..2b400ed 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,14 @@ User visible incompatible changes
>
>  Date        Modules         Changes
>
> +2008-09-24  filevercmp      New module containing FILEVERCMP function 
> comparing

Don't capitalize function names like that.  i.e., write filevercmp,
not FILEVERCMP.

> +                            version strings (and file names with version). 
> This
> +                            function is supposed to be replacement for

s/supposed/intended/

> +                            STRVERSCMP function. It has same parameters and

use lower case function names here, too.

> +                            return value as standard STRCMP function. It is
> +                            based on VERREVCMP function from dpkg and 
> improved
> +                            to deal better with file suffixes.
> +
>  2008-09-23  sys_socket      Under Windows (MinGW), the module now adds
>                              wrappers around Winsock functions, so that
>                              socket descriptors are now compatible with
> diff --git a/lib/filevercmp.c b/lib/filevercmp.c
...
> +#include <sys/types.h>
> +#include <stdlib.h>
> +#include <stdbool.h>

Add "stdbool" to the dependency list in modules/filevercmp.

> +#include <ctype.h>

> +/* function comparing version strings (and file names with version)
> +
> +   This function is supposed to be replacement for STRVERSCMP function. Same
> +   parameters and return value as standard STRCMP function.
> +
> +   It is based on verrevcmp function from dpkg and improved to deal better
> +   with file suffixes. */

It'd be good to put a more formal description of
what the function does, along the lines of what Bruno proposed.
And use lower case function names.

> +int
> +filevercmp (const char *s1, const char *s2)
> +{
> +  /* easy comparison to see if versions are identical */
> +  if (!strcmp (s1, s2))
> +    return 0;
> +
> +  /* "cut" file suffixes */
> +  const char *s1_pos = s1;
> +  const char *s2_pos = s2;
> +  const char *s1_suffix = match_suffix (&s1_pos);
> +  const char *s2_suffix = match_suffix (&s2_pos);
> +  size_t s1_len = (s1_suffix ? s1_suffix : s1_pos) - s1;
> +  size_t s2_len = (s2_suffix ? s2_suffix : s2_pos) - s2;
> +
> +  /* restore file suffixes if strings are identical after "cut" */
> +  if ((s1_suffix || s2_suffix) && (s1_len == s2_len) && 0 ==
> +      strncmp (s1, s2, s1_len))

split expressions before operators, not after:

     if ((s1_suffix || s2_suffix) && (s1_len == s2_len)
         && 0 == strncmp (s1, s2, s1_len))

> +  {
> +    s1_len = s1_pos - s1;
> +    s2_len = s2_pos - s2;
> +  }
> +
> +  int rc = verrevcmp (s1, s1_len, s2, s2_len);
> +  return (rc == 0)
> +    /* return 0 if (and only if) strings S1 and S2 are identical */
> +    ? strcmp(s1, s2) : rc;

strcmp(s1, s2) is always nonzero here, since
it was tested at the beginning of the function.

> +}
> +
> +/*
> +   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)
> +{
> +  const char *match = NULL;
> +  bool read_alpha = false;
> +  while (**str)
> +    {
> +      if (read_alpha)
> +        {
> +          read_alpha = false;
> +          if (!xisalpha (**str))
> +            match = NULL;
> +        }
> +      else if ('.'==**str)

use spaces around operators like "==":

         else if ('.' == **str)

I suggest that you filter the new .c file through GNU indent
and examine any differences that induces.

> +        {
> +          read_alpha = true;
> +          if (!match)
> +            match = *str;
> +        }
> +      else if (!xisalnum (**str))
> +        match = NULL;
> +      (*str)++;
> +    }
> +  return match;
> +}
> +
> +/* verrevcmp helper function */
> +inline int
> +order (unsigned char c)
> +{
> +  if (c == '~')
> +    return -1;
> +  else if (xisdigit (c))
> +    return 0;
> +  else if (xisalpha (c))
> +    return c;

Maybe reorder the above, to test the less-likely-to-be-true
condition (c == '~') later?

> +  else
> +    return c + 256;
> +}
> +
> +/* slightly modified ververcmp function from dpkg
> +   S1, S2 - compared string
> +   S1_LEN, S2_LEN - length of strings to be scanned */
> +static int
> +verrevcmp (const char *s1, size_t s1_len, const char *s2, size_t s2_len)
> +{
> +  int s1_pos = 0;
> +  int s2_pos = 0;

Why "int", and not size_t?

> +  while (s1_pos < s1_len || s2_pos < s2_len)
> +    {
> +      int first_diff = 0;
> +      while ((s1_pos < s1_len && !xisdigit (s1[s1_pos])) || (s2_pos < s2_len
> +         && !xisdigit (s2[s2_pos])))
> +     {
> +       int s1_c = (s1_pos == s1_len)
> +         ? 0 : order (s1[s1_pos]);
> +       int s2_c = (s2_pos == s2_len)
> +         ? 0 : order (s2[s2_pos]);
> +       if (s1_c != s2_c)
> +         return s1_c - s2_c;
> +       s1_pos++;
> +       s2_pos++;
> +     }
> +      while (s1[s1_pos] == '0')
> +     s1_pos++;
> +      while (s2[s2_pos] == '0')
> +     s2_pos++;
> +      while (xisdigit (s1[s1_pos]) && xisdigit (s2[s2_pos]))
> +     {
> +       if (!first_diff)
> +         first_diff = s1[s1_pos] - s2[s2_pos];
> +       s1_pos++;
> +       s2_pos++;
> +     }
> +      if (xisdigit (s1[s1_pos]))
> +     return 1;
> +      if (xisdigit (s2[s2_pos]))
> +     return -1;
> +      if (first_diff)
> +     return first_diff;
> +    }
> +  return 0;
> +}
> +
> diff --git a/lib/filevercmp.h b/lib/filevercmp.h
...
> +int filevercmp (const char *s1, const char *s2);
> +
> +#endif // FILEVERCMP_H

Don't use "//".  Use /* ... */ instead.

> diff --git a/m4/filevercmp.m4 b/m4/filevercmp.m4
> new file mode 100644
> index 0000000..5c78ea0
> --- /dev/null
> +++ b/m4/filevercmp.m4
> @@ -0,0 +1,7 @@
> +# filevercmp.m4
> +# TODO: license?
> +
> +AC_DEFUN([gl_FILEVERCMP],
> +[
> +  AC_LIBOBJ([filevercmp])
> +])

You may prefer to eliminate the filevercmp.m4 file and
instead add the single line AC_LIBOBJ([filevercmp]) to the
configure.ac section of the modules file.
If you do that, then remove m4/filevercmp.m4 from the Files: section,
too, of course.

> diff --git a/modules/filevercmp b/modules/filevercmp
> new file mode 100644
> index 0000000..bdbd6df
> --- /dev/null
> +++ b/modules/filevercmp
> @@ -0,0 +1,24 @@
> +Description:
> +function comparing version strings (and file names with version)
> +
> +Files:
> +lib/filevercmp.h
> +lib/filevercmp.c
> +m4/filevercmp.m4
> +
> +Depends-on:
> +string
> +
> +configure.ac:
> +gl_FILEVERCMP
> +
> +Makefile.am:
> +
> +Include:
> +"filevercmp.h"
> +
> +License:
> +GPL

Perhaps LGPL, since it's intended to be a strverscmp replacement?

> +Maintainer:
> +all
> diff --git a/modules/filevercmp-tests b/modules/filevercmp-tests
> new file mode 100644
> index 0000000..02554fe
> --- /dev/null
> +++ b/modules/filevercmp-tests
> @@ -0,0 +1,11 @@
> +Files:
> +tests/test-filevercmp.c
> +
> +Depends-on:
> +filevercmp
> +
> +configure.ac:
> +
> +Makefile.am:
> +TESTS += test-filevercmp
> +check_PROGRAMS += test-filevercmp

> diff --git a/tests/test-filevercmp.c b/tests/test-filevercmp.c
...
> +/* set of well sorted examples */
> +static const char *examples[] =

Looks like you can add a const here:

  static const char *const examples[] =

> +{
> +  "gcc-c++-10.fc9.tar.gz",
> +  "gcc-c++-10.8.12-0.7rc2.fc9.tar.bz2",
...

> +int
> +main (int argc, char **argv)
> +{
> +  /* Following tests taken from test-strverscmp.c */
> +  ASSERT (filevercmp ("", "") == 0);
> +  ASSERT (filevercmp ("a", "a") == 0);
> +  ASSERT (filevercmp ("a", "b") < 0);
> +  ASSERT (filevercmp ("b", "a") > 0);
> +  /* these results differ from strverscmp
> +  ASSERT (filevercmp ("000", "00") < 0);
> +  ASSERT (filevercmp ("00", "000") > 0); */

Rather than just commenting out these two, instead, assert
the correct-for-filevercmp condition and add a comment per line.

> +  ASSERT (filevercmp ("a0", "a") > 0);
> +  ASSERT (filevercmp ("00", "01") < 0);
> +  ASSERT (filevercmp ("01", "010") < 0);
> +  /* these results differ from strverscmp
> +  ASSERT (filevercmp ("010", "09") < 0);
> +  ASSERT (filevercmp ("09", "0") < 0); */

Likewise.

> +  ASSERT (filevercmp ("9", "10") < 0);
> +  ASSERT (filevercmp ("0a", "0") > 0);
> +
> +  /* compare each version string with each other - O(n^2) */
> +  const char **i;
> +  for (i = examples; *i; i++)
> +    {
> +      const char **j;
> +      for (j = examples; *j; j++)
> +     {
> +       int result = filevercmp (*i, *j);
> +       if (result < 0)
> +         ASSERT (i < j);
> +       else if (0 < result)
> +         ASSERT (j < i);
> +       else
> +         ASSERT (i == j);
> +     }
> +    }
> +
> +  return 0;
> +}
> +