Re: [RFC 1/9] Unify windows specifics into common/windows-hdep files

[Eli Zaretskii <eliz at gnu dot org>]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Wed, 30 Mar 2011 14:04:45 +0200
> 
>   This unifies windows path handling routines and
> macros related to use of ASCII or WIDE version of Windows API.
                           ^^^^^
You mean "ANSI", I presume.

> +/* The primary purpose of this file is to implement conversion between
> +   POSIX style and Windows NATIVE style path and path lists.

GNU coding standards frown on using "path" for anything but PATH-style
lists of directories.  You should use "file name" instead.

Same comment on the "path" part in the name of the functions you
coded.

> +int
> +windows_conv_path (conv_type oper, const void *from, void *to, int size)
> +{
> +#ifdef USE_MINGW_CONV
> +  if (size == 0)
> +    {
> +#ifdef USE_WIDE_WINAPI
> +      if (oper == WINDOWS_POSIX_TO_NATIVE_W)
> +	return MultiByteToWideChar (CP_ACP, 0, from, -1, to, 0);
> +      if (oper == WINDOWS_NATIVE_W_TO_POSIX)
> +	return WideCharToMultiByte (CP_ACP, 0, from, -1, to, 0, 0, 0);
> +      else
> +#endif /* USE_WIDE_WINAPI */

Hmm... I'm not sure I understand the intent.  Conversion from UTF-16
to the ANSI codepage is by definition lossy; what do you expect to
come out of this conversion in those cases?  And you don't even use
the WC_NO_BEST_FIT_CHARS flag (as MSDN says you should when converting
file names), nor check for errors with GetLastError.  It looks like
this can never work reliably, or am I missing something?

> +      int len = strlen((const char *) from) + 1;
> +#ifdef USE_WIDE_WINAPI
> +      if (oper == WINDOWS_POSIX_TO_NATIVE_W)
> +	return MultiByteToWideChar (CP_ACP, 0, from, len, to, size);
> +      if (oper == WINDOWS_NATIVE_W_TO_POSIX)
> +	return WideCharToMultiByte (CP_ACP, 0, from, len, to, size, 0, 0);
> +      else
> +#endif /* USE_WIDE_WINAPI */
> +        {
> +	  if (size < len)
> +	    len = size;
> +	  strncpy ((char *) to, (const char *) from, len);
> +	  if (oper == WINDOWS_NATIVE_TO_MSYS)
> +	    {
> +	      char * p;
> +	      p = strchr (to, '\\');
> +	      while (p)
> +		{
> +		  *p = '/';
> +		  p = strchr (to, '\\');
> +		}

Here, I don't understand why you flip the slashes only when `oper' is
something other that WINDOWS_POSIX_TO_NATIVE_W or
WINDOWS_NATIVE_W_TO_POSIX.  Why not flip the slashes together with
converting from multibyte to wide characters and back?

> +	      if (((char *) to)[1] == ':' && ((char *) to)[2] == '/')

Is it guaranteed that `to' is at least 2 characters long?  What if
it's only 1 character long?

> +      return cygwin_conv_to_full_posix_path (from, to);
> +      break;

Why do we need a `break' after a `return'?

> +/* Analogeous function for PATH style lists.  */
      ^^^^^^^^^^
A typo.

> +   GDB with wide char use even on systems for which the default is to
> +   use ASCII chars.  */
          ^^^^^
"ANSI".

> +#undef _G
> +/* Macro _G_SUFFIX(text) expands either to 'text "A"' or to 'text "W"'
> +   depending on the use of wide chars or not.  */
> +#undef _G_SUFFIX

I think the C Standard says that macros whose name begins with an
underscore and a capital letter are reserved.  Applications should not
use such macros.

> +   Otherwise mingw compiler is assumed, which defaults to use of ascii
> +   WINAPI for now (this may change later).                       ^^^^^

"ANSI".

> +   Default can be overridden either by defining USE_WIDE_WINAPI
> +   to force use of wide API, or by defining USE_ASCII_WINAPI to prevent
> +   use of wide API.  */

But do we actually support that?  AFAIK, to get the wide APIs, you
need to compile with _UNICODE defined.  But we don't have such
definitions anywhere, do we?

> +# define CreateProcess CreateProcessW
> +# define GetSystemDirectory GetSystemDirectoryW
> +# define windows_strlen wcslen

Ouch!  So any API that needs one of the two varieties will need to be
added to this list of #define's?  Is that wise?

> +typedef enum
> +{
> +  WINDOWS_NATIVE_A_TO_POSIX = 1
> +  ,WINDOWS_POSIX_TO_NATIVE_A = 2
> +#ifdef USE_WIDE_WINAPI
> +  ,WINDOWS_NATIVE_W_TO_POSIX = 3
> +  ,WINDOWS_POSIX_TO_NATIVE_W = 4
> +#endif
> +  ,WINDOWS_NATIVE_TO_MSYS = 5

I think this is ugly.  Why cannot you put the commas after the values?

> +   Returns size of converted string in characters or
> +   -1 if there was an error.  */
> +
> +extern int windows_conv_path (conv_type oper, const void *from,
> +			      void *to, int size);

The return value is not -1 when it fails, it's zero, because that's
what MultiByteToWideChar and WideCharToMultiByte return in that case.

> +/* Analogeous function for PATH style lists.  */
      ^^^^^^^^^^
A typo.