MADEAPPS / newton-dynamics

Newton Dynamics is an integrated solution for real time simulation of physics environments.
http://www.newtondynamics.com
Other
938 stars 182 forks source link

Potential improvements to LoadVisualStudioPlugins #214

Open JayFoxRox opened 4 years ago

JayFoxRox commented 4 years ago

On Windows, the check for enabling the plugin loader is:

https://github.com/MADEAPPS/newton-dynamics/blob/fd2c31db491cda38612649809c5f4341f7f7393a/sdk/dgPhysics/dgWorldPlugins.cpp#L43-L46

This is very confusing, because I don't see why the _MSC_VER would matter (even then I'd assume this landed >= 1700 or you probably want to check >= for the known release that added it [1800?]). I assume this is done to bring in a specific version of the Windows CRT for the use of _findfirst.

However, if _findfirst is the problem, then that's probably avoidable. That very same loop uses the Windows API (LoadLibrary and friends), so it could potentially use the FindFirstFileA function which has probably existed much longer than _findfirst. It would also reduce dependency on the MS CRT, which potentially allows for other compilers on Windows.

Due to this, it would also make sense to rename this function to LoadWindowsPlugins, as it would be compiler independent at that point (in addition to that, "Visual Studio" typically refers to the IDE, with "Visual C++" refering to the MS dialect). Checks would only have to be done for _WIN32, and no longer for _MSC_VER.

JulioJerez commented 4 years ago

yes that was a mistake, it is fixed now. Thanks

JayFoxRox commented 4 years ago

I'd still prefer it to be FindFirstFile (provided by Windows API, which comes with the OS) and check for _WIN32 so I could cross-compile with Clang on Linux, without the Microsoft proprietary C Runtime (comes with Visual C++ compiler; which provides _findfirst).

If this is not changed in newton-dynamics I'll probably just implement that part of the C runtime myself (for our toolchain) or I'll have to fork newton-dynamics to fix it for our original Xbox port. It's just a minor annoyance.

JulioJerez commented 4 years ago

ok I always thought _findfirst was the cross plaform call, but it turned out both are windows functions. No problem, I change to FindFirstFile, I take your word that is will compile with clang and other systems. get the latest for github

JayFoxRox commented 4 years ago

I reviewed the commit: You must use WIN32_FIND_DATA if you use FindFirstFile (either unicode or ASCII, depending on compiler setup); or you must use FindFirstFileA with WIN32_FIND_DATAA (always ASCII). You should also check for _WIN32 (all compilers compiling for Windows) instead of _MSC_VER (only MS compilers, regardless if compiling for Windows or otherwise)

ok I always thought _findfirst was the cross plaform call, but it turned out both are windows functions.

My understanding is this:

Because LoadLibrary is also a Windows function anyway, it's safe to use FindFirstFile here: the code will only run on Windows (or Windows-likes, like Xbox) anyway. Hence you should pick FindFirstFile, check for _WIN32 and include <windows.h>. If you want to force Unicode, you must use a W suffix, if you want to force ASCII you need to use an A suffix, and if you want to use the compiler default you don't use any suffix.