search

tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

Windows: don't check for MSVC for supporting Unicode filenames #236

Closed chris-se closed 4 months ago

chris-se commented 5 months ago

The corresponding wchar_t functionality is also supported by MinGW (at least somewhat recent versions thereof), so the check for _MSC_VER causes the MinGW build to only support the local codepage, and thus differ from the MSVC build, and also have the issue of not being able to access some files on the filesystem if they are not representable in the local codepage.

This drops the defined(_MSC_VER) and only keeps the _WIN32 check, because this is not a compiler feature, but rather a platform feature, so that check is the correct one.

This patch has been used in production successfully for quite a while with MinGW builds.

chris-se commented 5 months ago

I've updated the patch to use just 1.11.6, and once that's merged I'll create a new pull request with the 1.11.6 -> 1.10.6 (this will conflict, so I won't do that immediately)

Thanks!

chris-se commented 4 months ago

Note that HDF5 itself doesn't check for a specific compiler for the availability of those functions, it just assumes that on Windows that the Win32 API functions are available:

https://github.com/HDFGroup/hdf5/blob/49cce9173f6e43ffda2924648d863dcb4d636993/config/cmake/ConfigureChecks.cmake#L71

From a quick internet search using the compilers you mentioned, without having tested it, all appear to support _wfopen() at least in the current version (which is the only non-standard C runtime function used in this code) -- and I assume that compilers on Windows do support invoking the Windows API for MultiByteToWideChar(). LCC appeared to have missed _wfopen() in very old versions, but that was fixed in 2017.

Windows NT 4 (mid-1990's) was the first Windows version that supported Unicode filenames and the wchar_t APIs, and Windows XP (end of 2001, more than 22 years ago) was the first Windows version also targeted at consumers that supported these APIs. (I'll ignore 2000 here because it was never accepted by enough consumers to count.) I think that anybody developing a compiler that works on even somewhat modern Windows would provide access to these.

I personally therefore think the best option would be to simply guard by defined(_WIN32) and if someone then comes back and says "btw, my compiler XYZ doesn't work anymore", this could be updated to defined(_WIN32) && !defined( ... define for broken compiler ...) - instead of explicitly opting in to compilers that work, because the expectation should be that on modern Windows these APIs are simply available.

If you disagree here I can change the patch to explicitly add a whitelist for MSVC, MinGW, and clang for Windows, where I know for sure this is supported.

tbeu commented 4 months ago

If you disagree here

Certainly not. I am rather convinced again.

Thanks for your contibution!