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

Pierre Muller
2011-03-30T21:34:25Z

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