Re: [flac-dev] Patch to add Unicode filename support for win32 flac
Janne Hyvärinen wrote: Seems safe indeed. Attached an updated patch where metaflac works too. Janne, thanks for your hard work on this. Unfortunately this patch fails to apply for me. The problem is that all the Visual Studio files have CRLF newlines, while all the C files have just LF and the patch has only LF. I'm sure there must be a better way around this, but I haven't found it. Currently I'm opening the patch, selecting the VS patch chunks one by one and converting those chunks to CRLF before applying them. Obviously, this is a huge pain in the neck and there us a chance that I mess something up :-). I think it would help if you split patches like this in two; one with just C files and the other with just VS project files. I could then run unix2dos on the VS project patch file and hopefully get something that applies correctly first time. So, comments on the actual code: a) There's a couple of files and a directory call 'utf8_io' whereas I think they would be better called 'win_utf8' or 'win_utf8_io'. b) Similarly, the 'FLAC__STRINGS_IN_UTF8' should also have 'WIN' in it to make it clear that this is a windows feature. However, it would be nice if this worked for MinGW as well so this should probaby be: #if defined(_MSC_VER) || defined(__MINGW__) c) Not keen on the '_flac_stat' identifier. According to the ISO C standard, all identifiers starting with an underscore are reserved for the compiler implementor. Should probably be using flac_stat_ instead. d) Would be nice to reduce the #ifdef hackery. I'll have a look at this some more and probably hack the above myself. Then I'm commit it and get you to check it. Cheers, Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 19.3.2013 15:49, JonY wrote: On 3/19/2013 19:59, Janne Hyvärinen wrote: On 18.3.2013 12:25, Erik de Castro Lopo wrote: JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. +1 Erik Alright, here's a patch utilizing this function. There's a lot of changes here. Project files have a new configuration called Release (UTF8), intended to be used when building the command line tools. This project has the required UTF-8 define in place so all libraries expect things in encoded format. Regular Debug and Release configurations do not use any new tricks so existing projects won't break when compiled with those settings. I'm at work and couldn't do extensive testing, but command line FLAC.exe seems to perform everything right with this. Metaflac probably requires some minor tweaks but I wanted to show some progress so that 1.3 doesn't slip out the door while I'm busy. Should the following #if defined _MSC_VER || defined __MINGW32__ be simplified to #ifdef WIN32 ? Alternatively _WIN32. Since it's win32 and not something compiler specific. Indeed. Not sure what I was thinking. As for the macros: +#define flac_vfprintf vfprintf_utf8 +#define flac_fopen fopen_utf8 +#define flac_stat _stat64_utf8 ... I leave this up for Erik to decide, though I would have preferred not using rename macros at all. I think this is the sanest way. Only few #ifdefs instead of wrapper functions filled with them that do essentially nothing on *nix. Not sure if these were intended: +#include sys/stat.h /* for flac_stat() */ +#include sys/utime.h /* for flac_utime() */ +#include io.h /* for flac_chmod() */ Nope. Bug from mass replace. As for calling __wgetmainargs, I have some concerns about the security implications: LoadLibrary(msvcrt.dll) - Which msvcrt? Theoretical security exploit. There is msvcrt.dll in the System32 dir in all supported Windows systems. That is what the function targets, but of course LoadLibrary searches from exe's dir first. I think security exploit concerns are warrantless, if you can place malicious replacement c-runtime dll in the exe's path you have already won. I think it is best to link it directly, please use the following prototype and call it directly: = #ifdef _DLL #define CALL_DLLIMPORT __declspec(dllimport) #else #define CALL_DLLIMPORT #endif int __cdecl CALL_DLLIMPORT __wgetmainargs(int*, wchar_t***, wchar_t***, int, int*); = This should simplify the error handling logic and help against LoadLibrary handle leaks, though the leak should not be an issue in practice since it is only called once. The symbol should also be present in MSVCR* DLLs. This alone does nothing. It requires linking with an object file that then deals with the function. If we link against msvcrt.lib the flac.exe binary will no longer be static and it won't work without external runtimes (which would also be loaded from the exe's dir if they exist there). Linking with msvcmrt.lib won't find the function and unicode version msvcurt.lib causes this error: Error1error LNK2005: ___iob_func already defined in msvcurt.lib(MSVCR110.dll)G:\test\LIBCMT.lib(_file.obj)test Error2error LNK1169: one or more multiply defined symbols foundG:\test\Release\test.exetest In stat_utf8, dealing with 32bit/64bit time_t? The following check should really compile time rather than runtime: sizeof(*buffer) == sizeof(st) Entire stat_utf8 function can be removed. The code was changed later to always use stat64 one. Though I'd assume compiler to optimize sizeof checks appropriately. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/20/2013 00:35, Janne Hyvärinen wrote: As for calling __wgetmainargs, I have some concerns about the security implications: LoadLibrary(msvcrt.dll) - Which msvcrt? Theoretical security exploit. There is msvcrt.dll in the System32 dir in all supported Windows systems. That is what the function targets, but of course LoadLibrary searches from exe's dir first. I think security exploit concerns are warrantless, if you can place malicious replacement c-runtime dll in the exe's path you have already won. Yeah, which is why I said it was theoretical. I've seen code that use __ImageBase to over the import tables to find out which MSVCR* DLL is used and use GetModuleHandleA to avoid LoadLibrary. I think it is best to link it directly, please use the following prototype and call it directly: = #ifdef _DLL #define CALL_DLLIMPORT __declspec(dllimport) #else #define CALL_DLLIMPORT #endif int __cdecl CALL_DLLIMPORT __wgetmainargs(int*, wchar_t***, wchar_t***, int, int*); = This should simplify the error handling logic and help against LoadLibrary handle leaks, though the leak should not be an issue in practice since it is only called once. The symbol should also be present in MSVCR* DLLs. This alone does nothing. It requires linking with an object file that then deals with the function. If we link against msvcrt.lib the flac.exe binary will no longer be static and it won't work without external runtimes (which would also be loaded from the exe's dir if they exist there). Linking with msvcmrt.lib won't find the function and unicode version msvcurt.lib causes this error: Error1error LNK2005: ___iob_func already defined in msvcurt.lib(MSVCR110.dll)G:\test\LIBCMT.lib(_file.obj)test Error2error LNK1169: one or more multiply defined symbols foundG:\test\Release\test.exetest There is no __wgetmainargs in the static libcmt? Interesting. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
Brian Willoughby wrote: I believe that shell does handle wildcards on all Unix variants, including OSX. Yes. Since Windows does not handle them, I suggest that the main flac code not be littered with code that's not necessary on the primary platforms. No, the flac code will not be littered with code that's not necessary on the primary platforms. There will be some windows specific code in a new file, a bunch of replacements of existing fopen() calls with flac_fopen() (similarly for chmod and utime) and the main function for the flac and metaflac executables will have an additional: #ifdef _WIN32 if (!convert_argv_to_utf8(argc, argv)) flac__utils_printf(stderr, 1, ERROR: yada yada\n); #endif This is a small un-obtrusive change that I fully support. I would however like to see it sooner rather than later so we can get this damn thing released :-). Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/18/2013 14:55, Erik de Castro Lopo wrote: Brian Willoughby wrote: I believe that shell does handle wildcards on all Unix variants, including OSX. Yes. Since Windows does not handle them, I suggest that the main flac code not be littered with code that's not necessary on the primary platforms. No, the flac code will not be littered with code that's not necessary on the primary platforms. There will be some windows specific code in a new file, a bunch of replacements of existing fopen() calls with flac_fopen() (similarly for chmod and utime) and the main function for the flac and metaflac executables will have an additional: #ifdef _WIN32 if (!convert_argv_to_utf8(argc, argv)) flac__utils_printf(stderr, 1, ERROR: yada yada\n); #endif This is a small un-obtrusive change that I fully support. I would however like to see it sooner rather than later so we can get this damn thing released :-). Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/18/2013 09:55, LRN wrote: On 18.03.2013 02:10, JonY wrote: On 3/17/2013 23:01, LRN wrote: All those ifdefs will at least be confined rather than spread out through the code. I did it plibc-style: in compat.h: #if defined(_WIN32) #define FOPEN grabbag__fopen_utf8_wrapper #else #define FOPEN fopen #endif in grabbag: #if defined(_WIN32) implement grabbag__fopen_utf8_wrapper, which has the same signature as fopen, but does utf8-utf16 conversion internally, then calls wfopen #endif and replace fopen with FOPEN everywhere else. Don't do that, it leaks into the system headers How? compat.h is not a public header, it is only used internally in FLAC. And i don't think that system headers have defines for FOPEN and such. Preprocessor macros are not scoped. Even if FOPEN may not, but future compat code that follow this pattern might easily bump into unrelated identifiers. utime_uft8 already did some amount of damage. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 18.3.2013 11:35, JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. MSVC also comes with http://msdn.microsoft.com/en-us/library/8bch7bkk%28v=vs.80%29.aspx. To support unicode with these methods would require somewhat more #ifdef code in main. We'd need to change project files to define Unicode character set and turn main into _wmain and char *argv to wchar_t *argv. Also these are MSVC's internal features, if I'm not mistaken. Other compilers on Windows would then require different solutions. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/18/2013 19:21, Janne Hyvärinen wrote: On 18.3.2013 11:35, JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. MSVC also comes with http://msdn.microsoft.com/en-us/library/8bch7bkk%28v=vs.80%29.aspx. To support unicode with these methods would require somewhat more #ifdef code in main. We'd need to change project files to define Unicode character set and turn main into _wmain and char *argv to wchar_t *argv. Also these are MSVC's internal features, if I'm not mistaken. Other compilers on Windows would then require different solutions. For setargv.obj, it is equivalent to doing int _dowildcard = 1; in any linked object on mingw. For wmain, regular mingw does not support it, but mingw-w64 does. The issue is that the former is far more common in the wild. I still prefer the __wgetmainargs approach, the exact same code can be used for both mingw and msvc. We don't need to care about 9x, I think. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 18.03.2013 13:35, JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. I can't find its version info. MSDN only documents it as far as VS2010 (normal C functions are documented as far as VS2003), and it's not present in any header file i have. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRRvvBAAoJEOs4Jb6SI2CwkX8IALcXD6FV5G8Mdg6G0ORajgbj tveYgs7d7FUKZSB8zo9L4GmodfzqacnxYnAbl6G+5+6zMgb5MjBbJPV5Z5LvCFrI OPk067seyK3ZY2YZuv6RFwOn3V0e4y1WnCa4h0eep1gkRuhIO5QXFeqtRVucys4E U5Xcp61JcheAhIJf8dLvUn/C7DkZFTtrdoJnRxyzNSqQoOyGSb+aocIP/LADapFJ QSqjSfXak4s6IGSro9lod9l6Au4/L1LxD6rS6JlNS+Yuy/tzJtsL6ANe4ULSFoVz W8iZ9wMgHAK982GdyGJO76UmCe8p5N4KLUjPnOItLZDYvZxSHN/AkGY0xaK9388= =cgor -END PGP SIGNATURE- ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/18/2013 19:34, LRN wrote: On 18.03.2013 13:35, JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. I can't find its version info. MSDN only documents it as far as VS2010 (normal C functions are documented as far as VS2003), and it's not present in any header file i have. I don't think it is, you probably required to declare it yourself. The symbol is in the MSVCRXX runtime dll, all the way from MSVCRT.dll to MSVC2012 MSVCR110. Suffice to say it is always there. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 18.3.2013 15:17, JonY wrote: On 3/18/2013 19:34, LRN wrote: On 18.03.2013 13:35, JonY wrote: Before anyone does anything, see __wgetmainargs http://msdn.microsoft.com/en-us/library/ff770599.aspx. It can expand wildcards. Since it already provides argc/argv/env, it is more a less a drop-in replacement for the main() arguments. I can't find its version info. MSDN only documents it as far as VS2010 (normal C functions are documented as far as VS2003), and it's not present in any header file i have. I don't think it is, you probably required to declare it yourself. The symbol is in the MSVCRXX runtime dll, all the way from MSVCRT.dll to MSVC2012 MSVCR110. Suffice to say it is always there. Seems you are correct. I just did some testing and the unicode version appears usable from ansi program too. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
JonY wrote: On 3/17/2013 10:33, Janne Hyvärinen wrote: Here's a patch that makes MSVC compiled flac.exe able to use wildcards and encode/decode files with Unicode characters in names. It may not be the prettiest code but it fulfills its primary purpose. I tried to alter FLAC code as little as possible. It replaces argv with utf-8 encoded version and only converts to usable Unicode for file functions. That means printed texts and tags that contain non-ascii characters will be broken, but it is fixable if these changes are acceptable. Can this be reworked without the defines? This way, it should support mingw builds too. +1 I'd like to see some way to do this to: a) Make this usable from MinGW compiled binaries. b) Reduce the the amount of inline #ifdef hackery. Cheers, Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 17.03.2013 06:33, Janne Hyvärinen wrote: Here's a patch that makes MSVC compiled flac.exe able to use wildcards and encode/decode files with Unicode characters in names. It may not be the prettiest code but it fulfills its primary purpose. I tried to alter FLAC code as little as possible. It replaces argv with utf-8 encoded version and only converts to usable Unicode for file functions. That means printed texts and tags that contain non-ascii characters will be broken, but it is fixable if these changes are acceptable. /me looks at chmod and utime wrappers: Ah, i knew i've missed something! :) Also, i didn't consider wildcards (i thought shell was supposed to handle them...). -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRRaGLAAoJEOs4Jb6SI2CwapIIANAihJPXnq5rJDXIs1JdJvUx rDk3c1obe8PTE2ur+jU86TDgflIotY1mYCUptglNr28IPiWs1Nj7qKM0sIXfa6e7 kaAocJStwXNjA/VCLUXd4Ka+OY0MOdKnldAcepLnTPBZSgKXVHOoe/A98tHc6BsS 1+fBKoU6YFb7jubaW6P2IVquu/rhbCyAI/3+3yaBdMYkgvs5zq/HT/iUYWjVD5V5 jTW/lDFNzqRydIPH3EdB64W9kXAXhLi75P3DVD+4OtDW9eTiLIIYWNxh0Bbk6AWg f/AP9kDp/2qSg5QSab0j5fEQDYG6RnYl4MNXLhwhLRyHK19WlBLucw7l/4ibdWs= =2RLE -END PGP SIGNATURE- ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 17.03.2013 18:55, JonY wrote:
On 3/17/2013 18:37, Erik de Castro Lopo wrote:
JonY wrote:
On 3/17/2013 10:33, Janne Hyvärinen wrote:
Here's a patch that makes MSVC compiled flac.exe able to use
wildcards and encode/decode files with Unicode characters in
names. It may not be the prettiest code but it fulfills its
primary purpose. I tried to alter FLAC code as little as
possible. It replaces argv with utf-8 encoded version and
only converts to usable Unicode for file functions. That
means printed texts and tags that contain non-ascii
characters will be broken, but it is fixable if these changes
are acceptable.
Can this be reworked without the defines? This way, it should
support mingw builds too.
+1
I'd like to see some way to do this to:
a) Make this usable from MinGW compiled binaries. b) Reduce the
the amount of inline #ifdef hackery.
A simple way to do this is to make all C IO calls that need OS
specific hacks go into the compatibility module. Eg for open()
calls:
int _flac_open(){ #if win32 ... #else if os2 ... #else ...
[generic call to open() without hacks] #endif }
All those ifdefs will at least be confined rather than spread out
through the code.
I did it plibc-style:
in compat.h:
#if defined(_WIN32)
#define FOPEN grabbag__fopen_utf8_wrapper
#else
#define FOPEN fopen
#endif
in grabbag:
#if defined(_WIN32)
implement grabbag__fopen_utf8_wrapper, which has the same signature
as fopen, but does utf8-utf16 conversion internally, then calls wfopen
#endif
and replace fopen with FOPEN everywhere else.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJRRdrIAAoJEOs4Jb6SI2CwL4YH/Ayixx9r3XitYI0j1FH+xQd3
lrhJ3I3Di1uE0/LES9/mL65T6yBvwov1LDoL4JMHMK4/mH78s0wDZXYr6h/FoEhW
8XpxRL6ujDfCEb2cp76KAcZLL3rcO4uEsgVJsXBtkC5K+zpjzgMVnasg+oH/Z6sS
PdqPg/w+40FOgkYszMUwwJJJF/y2uTZXSOZoDyM8glx2oMhrAHmi0Yx7ksehndSo
wN6JaJFe3IdprOZdyLXpxsPxiNt1MiR0b7BKg4bAFt8evStcD9d/CHSJ3te5134v
d4YswJj/GQ1y9fJLpW+wPSEYmcJbi+Brkfo8OYs5pcs2Tsv0ZfQ80ItoJZ91CyQ=
=ZnwK
-END PGP SIGNATURE-
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/17/2013 23:01, LRN wrote: All those ifdefs will at least be confined rather than spread out through the code. I did it plibc-style: in compat.h: #if defined(_WIN32) #define FOPEN grabbag__fopen_utf8_wrapper #else #define FOPEN fopen #endif in grabbag: #if defined(_WIN32) implement grabbag__fopen_utf8_wrapper, which has the same signature as fopen, but does utf8-utf16 conversion internally, then calls wfopen #endif and replace fopen with FOPEN everywhere else. Don't do that, it leaks into the system headers and breaks mingw if FLAC_USE_FOPEN_UTF8 is defined. Call the wrappers directly instead of using a macro. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
JonY wrote: On 3/17/2013 23:01, LRN wrote: All those ifdefs will at least be confined rather than spread out through the code. I did it plibc-style: in compat.h: #if defined(_WIN32) #define FOPEN grabbag__fopen_utf8_wrapper #else #define FOPEN fopen #endif in grabbag: #if defined(_WIN32) implement grabbag__fopen_utf8_wrapper, which has the same signature as fopen, but does utf8-utf16 conversion internally, then calls wfopen #endif and replace fopen with FOPEN everywhere else. Don't do that, it leaks into the system headers and breaks mingw if FLAC_USE_FOPEN_UTF8 is defined. Call the wrappers directly instead of using a macro. +1 Yep, I prefer not to have too much #ifdef hackery. In my recent replacement of all the sprintf/_snprintf stuff, I relaced all the calls with a call to flac_snprintf() and localised #ifdef hackery to the implementation of that function. From a patch cleanliness POV, I like to see the new functionality added in one patch and a separate patch to change all the old function usage to the new function usage. For example, in commit 06af237c I added the new flac_snprintf() function and in commit 3c84f9e8 I replaced all the old calls to sprintf/_snprintf. Cheers, Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 18.03.2013 02:10, JonY wrote: On 3/17/2013 23:01, LRN wrote: All those ifdefs will at least be confined rather than spread out through the code. I did it plibc-style: in compat.h: #if defined(_WIN32) #define FOPEN grabbag__fopen_utf8_wrapper #else #define FOPEN fopen #endif in grabbag: #if defined(_WIN32) implement grabbag__fopen_utf8_wrapper, which has the same signature as fopen, but does utf8-utf16 conversion internally, then calls wfopen #endif and replace fopen with FOPEN everywhere else. Don't do that, it leaks into the system headers How? compat.h is not a public header, it is only used internally in FLAC. And i don't think that system headers have defines for FOPEN and such. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRRnP7AAoJEOs4Jb6SI2CwxWoH/13eGOZb62iAm4SzkDFQ6/Bh 2Goz3lz+9fK4Gq+tXyfPmXC1JabLdM1vnw43QgkAVWr3OrJ2+AN+LHocP9+YCYG4 ckd5Eisi32taDr3+CnaJzOrYQnaeD926iPC2vQoVOMIniGWTRVzIIYxood0gGXd3 oa5hPMvq1t/TXyKudSt8Jimeoe6vyoaLcBJCMykn9B5qh//ryiajGJlwQgicessb Rzw0/VgbLcck3XLyzm7gfsXxiYhRjeSalZyPxYw6DE8rsARxswk1TfWLB7faPAiI spaA2mEX7iQz9GPmKlhil/Q/rzsn3vt8lgHbC+WDD0843kkaC3MWPvPdqqsbFh4= =3u0R -END PGP SIGNATURE- ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 18.03.2013 02:37, Erik de Castro Lopo wrote: JonY wrote: On 3/17/2013 23:01, LRN wrote: All those ifdefs will at least be confined rather than spread out through the code. I did it plibc-style: in compat.h: #if defined(_WIN32) #define FOPEN grabbag__fopen_utf8_wrapper #else #define FOPEN fopen #endif in grabbag: #if defined(_WIN32) implement grabbag__fopen_utf8_wrapper, which has the same signature as fopen, but does utf8-utf16 conversion internally, then calls wfopen #endif and replace fopen with FOPEN everywhere else. Don't do that, it leaks into the system headers and breaks mingw if FLAC_USE_FOPEN_UTF8 is defined. Call the wrappers directly instead of using a macro. +1 Yep, I prefer not to have too much #ifdef hackery. In my recent replacement of all the sprintf/_snprintf stuff, I relaced all the calls with a call to flac_snprintf() and localised #ifdef hackery to the implementation of that function. From a patch cleanliness POV, I like to see the new functionality added in one patch and a separate patch to change all the old function usage to the new function usage. For example, in commit 06af237c I added the new flac_snprintf() function and in commit 3c84f9e8 I replaced all the old calls to sprintf/_snprintf. /me shrugs Sure, that'll work too. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRRnSAAAoJEOs4Jb6SI2CwPmoIAIM2pFUjl+TNDnPkDCBkLWPy zf58O5s8R+2S1uNfpd8fQrheZKdK8kSitJj9XYGELo+POyn8pJGEBjYvZyqVls88 gAHaE5wWgV5aSYmHcwQD8aXCB2LP9Lkpk01rvt6lZ9grw15nWUyhmiPd3uyQYwOD ZGrqAS661d4XoSP5N0hi86mUf9NcM6xXacDY+leRnSNowIrtbnCR0vhQJoHMpY7U fWB6dM4YEgZmCbj6he9ZHbXemlMiK+rRkoEsPPcAobGCIga6I0il6j/3JxI8bKCl VF+4Iz0APw22/+ck6K+tfde/9JGraZT2HsYm2PNAcPiuZPmvP0oTvQrGLkRJzr0= =GIQw -END PGP SIGNATURE- ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On Mar 17, 2013, at 03:57, LRN wrote: /me looks at chmod and utime wrappers: Ah, i knew i've missed something! :) Also, i didn't consider wildcards (i thought shell was supposed to handle them...). I believe that shell does handle wildcards on all Unix variants, including OSX. Since Windows does not handle them, I suggest that the main flac code not be littered with code that's not necessary on the primary platforms. Aren't Windows users accustomed to this feature being missing anyway? Brian ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Patch to add Unicode filename support for win32 flac
On 3/17/2013 10:33, Janne Hyvärinen wrote: Here's a patch that makes MSVC compiled flac.exe able to use wildcards and encode/decode files with Unicode characters in names. It may not be the prettiest code but it fulfills its primary purpose. I tried to alter FLAC code as little as possible. It replaces argv with utf-8 encoded version and only converts to usable Unicode for file functions. That means printed texts and tags that contain non-ascii characters will be broken, but it is fixable if these changes are acceptable. Can this be reworked without the defines? This way, it should support mingw builds too. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
