openexr icon indicating copy to clipboard operation
openexr copied to clipboard

restrict type qualifier

Open fnordware opened this issue 9 months ago • 7 comments

In 22cbb7966fa16dae2a1dde14c3a7d9966661516c the restrict type qualifier was added to some variables, but Visual Studio doesn't like it. Microsoft uses __restrict instead.

I guess we should make a RESTRICT macro to use in its place. I'd provide a pull request, but not sure where such a thing should live.

fnordware avatar Mar 08 '25 08:03 fnordware

bah, that's kinda ridiculous, that has been in the standard since C99, so it's hardly a "new" thing. That says to me that visual studio is not compiling that core code as C, but rather C++ (which does not officially recognise restrict as it has different strict aliasing rules), which is disappointing to say the least.

FYI, it does make some components a bit faster, so worth it. There is a system specific header, will clean that up (it's not in a generic place at the moment) and use a macro.

[edit: I should have mentioned, this is only used internally, not in the interface or involving external buffers as that can be a source of issues]

kdt3rd avatar Mar 10 '25 08:03 kdt3rd

Dug a little deeper and in Visual Studio 2019 and later it is possible to specify which C standard to use. It defaults to C89, but you can choose C11 or C17, and then restrict works.

If we say Visual Studio 2019 is the minimum version we support, then I guess we still have to get that /std:c11 flag to come out of CMake.

fnordware avatar Mar 10 '25 16:03 fnordware

I notice you picked c17 for your fork, assuming @kdt3rd is cool with that maybe we can go with your patch?

meshula avatar Mar 12 '25 05:03 meshula

The Microsoft docs say "C17 is largely a bug-fix release of ISO C11", so I figured go with that. Seems unlikely we would notice any practical difference.

Just waiting for the powers that be to decide to go this route vs. accommodate compilers that don't know about restrict.

fnordware avatar Mar 12 '25 05:03 fnordware

c11 / c17 is fine. I still have a pending todo to add detection and revert to __restrict. I AM setting c_std_11 on the library define (see OpenEXRLibraryDefine.cmake), I wonder why that isn't turning on the /std:c11 for msvc... [feel free to update that to 17 if that causes the flag to pass through]

kdt3rd avatar Mar 14 '25 23:03 kdt3rd

Hmm, yeah c_std_11 doesn't seem to be getting through. Changing it to c_std_17 didn't help. C++17 is getting set in every project with C++ files, but the C language standard is left at the default.

BTW, in Xcode it appears the dialect isn't getting set for C or C++, but Apple (or gcc) moves their compiler defaults along regularly, so it's not a problem. Point is, CMake might not be so good about applying that tag on all platforms, and that C language standard setting is relatively new to Visual Studio.

I don't really know CMake, or I'd try to do some more debugging.

fnordware avatar Mar 15 '25 00:03 fnordware

As you can see in b25f5e25e1535550e08509f4d1c20c2d11d5234f, adding that line from OpenEXRLibraryDefine.cmake directly into OpenEXRCore/CMakeLists.txt does work. Not sure why it's not working otherwise, but I'm a CMake newb.

fnordware avatar Mar 15 '25 05:03 fnordware