Kamil Dudka <[EMAIL PROTECTED]> 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:
