From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24630 invoked by alias); 30 Mar 2011 19:38:48 -0000 Received: (qmail 24474 invoked by uid 22791); 30 Mar 2011 19:38:45 -0000 X-SWARE-Spam-Status: No, hits=-0.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_SOFTFAIL,TW_CP X-Spam-Check-By: sourceware.org Received: from mtaout22.012.net.il (HELO mtaout22.012.net.il) (80.179.55.172) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 Mar 2011 19:38:37 +0000 Received: from conversion-daemon.a-mtaout22.012.net.il by a-mtaout22.012.net.il (HyperSendmail v2007.08) id <0LIV00700Y9QOG00@a-mtaout22.012.net.il> for gdb-patches@sourceware.org; Wed, 30 Mar 2011 21:38:35 +0200 (IST) Received: from HOME-C4E4A596F7 ([77.126.47.180]) by a-mtaout22.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LIV007ZEYK96T90@a-mtaout22.012.net.il>; Wed, 30 Mar 2011 21:38:34 +0200 (IST) Date: Wed, 30 Mar 2011 19:47:00 -0000 From: Eli Zaretskii Subject: Re: [RFC 1/9] Unify windows specifics into common/windows-hdep files In-reply-to: <00a201cbeed2$a924dfa0$fb6e9ee0$%muller@ics-cnrs.unistra.fr> To: Pierre Muller Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83lizwqtz9.fsf@gnu.org> References: <00a201cbeed2$a924dfa0$fb6e9ee0$%muller@ics-cnrs.unistra.fr> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-03/txt/msg01206.txt.bz2 > From: "Pierre Muller" > 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.