Re: [PATCH] add new module filevercmp

[Bruno Haible]

Hello Kamil,

Another set of comments:

> There are still opened some licensing issues as we are now waiting for reply 
> from original author.

Yes, we need to wait for this to be clear first.


> Could somebody provide me with a complete set of gcc parameters to catch all 
> unportable code in this module?

"gcc -Wall" is not bad, but to catch _all_ unportable code you need some pairs
of eyes to look at the code.


In filevercmp.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. */

This description is wishy-washy. Such a documentation comment should say
what are the arguments (not just reference to another function), and
what is the return value (likewise).

It should _not_ say how the function can be used, if more uses are possible.
(I can also use filevercmp if I have never heard of strverscmp. So there's
no need to mention strverscmp.)

It should also _not_ say how the function is implemented and its history.
That is irrelevant to the user. If you are proud to explain the history of
this function, the filevercmp.c file is the better place.

But the description should give an indication about how the comparison is
performed (abstractly, not procedurally). If you want to preserve the freedom
to change it later on, write something like this:
  "This function compares strings, in a way that if VER1 and VER2 are
   version numbers and PREFIX and SUFFIX are strings" [any strings?]
   "then VER1 < VER2 implies filevercmp (PREFIX VER1 SUFFIX, PREFIX VER2 SUFFIX)
    < 0.
   In the current implementation, the argument strings are dissected into ...
   and the pieces are compared as follows ..."


In filevercmp.m4:
> +AC_DEFUN([gl_FILEVERCMP],
> +[
> +  AC_LIBOBJ([filevercmp])
> +])

You don't need a .m4 file for this. You can even leave the configure.ac
section of the module description empty. Just add to the Makefile.am section
of the module description one line:

  lib_SOURCES += filevercmp.c


In filevercmp.c:

> +#include <sys/types.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <ctype.h>
> +
> +#include "filevercmp.h"

First, you need to include <config.h>. Then, it's good practice to include
"filevercmp.h" immediately after config.h. This verifies that it is self-
contained. (This is done correctly in the tests file, but sometimes we don't
have a unit test and still want to ensure the header file is ok.)

> +#define xisalnum(c) isalnum ((unsigned char) c)

You need to parenthesize the references to the macro argument:
                       isalnum ((unsigned char) (c))
Otherwise when c is an additive expression, for example, the cast applies
only to part of the expression.

> +#define xisdigit(c) isdigit ((unsigned char) c)
> +#define xisalpha(c) isalpha ((unsigned char) c)

In GNU style code, uppercase identifiers are used for macros. Yes, it is
a bit stupid for function-like macros that could also be defined as inline
functions, but it's the convention.

> +static int verrevcmp (const char *s1, size_t s1_len, const char *s2, size_t
> +    s2_len);
> +static const char * match_suffix (const char **str);

You don't need these forward declarations if you define the functions before
they are used. I.e. reorder the functions in a bottom-up way. But that's not
everyone's taste - often people prefer to think top-down.

> +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;

Some systems still don't have C99 compilers. Therefore you cannot use
variable declarations after statements. Either collect the variable
declarations at the top, or introduce blocks { ... } around the desired
scopes of the variables.

> +inline int
> +order (unsigned char c)

This is C, not C++. 'inline' alone is not portable. Use 'static inline'
instead; this is portable. Still, you need to add
  AC_REQUIRE([AC_C_INLINE])
to the .m4 file or to the configure.ac section of the module description.

> +    return c + 256;

256 is meant to be UCHAR_MAX + 1, here, right? Then better write UCHAR_MAX + 1.
UCHAR_MAX is defined in <limits.h>.

> --- 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
> +                            version strings (and file names with version). 
> This
> +                            function is supposed to be replacement for
> +                            STRVERSCMP function. It has 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.

A new module with a new function is not an incompatible change and therefore
does not need to be mentioned in the NEWS file. (Except if you are exceedingly
proud of it :-)).

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

To comment out several lines of code, it's better to use #if 0. It's more
immediately clear where the end of the disabled lines is, for people who
are not using an editor with syntax coloring.

Bruno