Kamil Dudka <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> 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;
> +}
> +
