Re: [PATCH 2/3] canonicalize-lgpl: Add basic tests for Mingw and first useful implementation.

[Bruno Haible]

Hi Jan,

> 2011-02-25  Jan Nieuwenhuizen  <address@hidden>
> 
>       * lib/canonicalize-lgpl.c (__realpath)[WINDOWS]: Add an
>       implementation for Mingw to pass newly added tests.
> 
>       * tests/test-canonicalize-lgpl.c (main)[WINDOWS]: Add test cases
>       for Mingw, checking basic sanity.

Thanks for providing the unit test that shows what you expect from this
function on native Windows platforms.

I was nearly going to commit your patch in your name, leaving the fine
points for discussion and tweaking afterwards, when I found out a couple
of points that need to be fixed before it can be committed:

  - In gnulib, we don't use tabs in .h and .c files. To untabify, you can
    use the 'expand' program from GNU coreutils. For how to avoid to
    introduce tabs in the future, please see the section "Indent with spaces,
    not TABs" in gnulib/README.

  - The functions slashify, backslashify, strlower take a 'const char *'
    parameter, but then destructively modify the argument. This is a no-no.
    If a function modifies a string argument, this argument must be typed
    'char *', and you must ensure that this causes no warnings with "gcc -Wall".

  > +    sname = malloc (PATH_MAX);
  > +    if (sname == NULL)
  > +      goto error;
  > +    strcpy (sname, name);
  - Here you are copying a string of unknown length into an array of size
    PATH_MAX, using strcpy. This is also a no-no. Please follow the advice
    from the GNU standards, section "Writing Robust Programs"
    <http://www.gnu.org/prep/standards/html_node/Semantics.html.

The fine points are:

  - In lib/canonicalize-lgpl.c you use the preprocessor expression
       (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
    4 times. The code would be easier to read if you did
       #if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
       # define WIN32_NATIVE
       #endif
    and then use 'defined WIN32_NATIVE' 4 times, like it is done in poll.c
    and many other files.

  - I see no point in having GL_PREFER_SLASH_INTERNALLY. Mingw's getcwd()
    function returns real Windows-style file names, like
       C:\cygwin\home\bruno
    or
       c:\Dokumente und Einstellungen\Default User
    therefore canonicalize_file_name should IMO do the same.

Bruno
-- 
In memoriam Stephan Brassloff <http://de.wikipedia.org/wiki/Stephan_Brassloff>