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

["Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>]

> -----Message d'origine-----
> De?: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé?: mercredi 30 mars 2011 21:39
> À?: Pierre Muller
> Cc?: gdb-patches@sourceware.org
> Objet?: Re: [RFC 1/9] Unify windows specifics into common/windows-hdep
files
> 
> > 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.

  Yes you are right, I should use the Microsoft API wording of
ANSI versus Unicode.
 
> > +/* 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.

  I have no opinion on that subject,
I just wanted to stay close to the cygwin function names...
 
> 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?

  I didn't check those options.
I will need to read their descriptions 
more precisely.
 
> > +      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?

 I agree that I also should do the flips on the two above cases...

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

  One more missing check, thanks for noticing. 
 
> > +      return cygwin_conv_to_full_posix_path (from, to);
> > +      break;
> 
> Why do we need a `break' after a `return'?
 Missing C code practice :( 
> > +/* Analogeous function for PATH style lists.  */
>       ^^^^^^^^^^
> A typo.

  Apparently, at least I was consistent ...
 
> > +   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.

  But we are also using __USEWIDE before my patch ...
 or do you mean that two underscores are OK?

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

  I completely remove __USEWIDE and tried to replace it by this new
USE_WIDE_WINAPI.
  Or is this __USEWIDE macro used anywhere else than inside GDB code?

  Should I rather call the macro USE_UNICODE_WINAPI
 (and USE_ANSI_WINAPI to force ANSI version?) 
 
> > +# 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?

  Isn't it better than being forced to use
#ifdef __USEWIDE
  CreateProcessW (...
#else
  CreateProcessA (...
#endif
 
> > +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?

  No reason, it's just that I also find it ugly to have a comma
after the last enum (but I am not even forced to do that here, so my
argument is bad...)
 
> > +   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.

  Here you are talking about the Mingw version:
I agree that I must check failure of those calls
and return -1 in that case.
 
> > +/* Analogeous function for PATH style lists.  */
>       ^^^^^^^^^^
> A typo.
Agreed

Thanks for the useful comments.

Pierre