[PATCH] Unify script conversions

[Yves]

Because it was historically grown, we have some duplicated code for converting script types to native types. This patch removes the file JSConversions.cpp and moves some code to ScriptConversions.cpp. The places using JSConversions.cpp are changed to use the ScriptInterface's conversion functions in ScriptConversions.cpp.

I also removed JSInterface_Vector3D because it had additional requirements to the conversion code that no other code has and because it's currently not used. I think it doesn't make sense to maintain code just because it could possibly be used again later.

One tricky bit is the conversions of the types like "unsigned" with unspecified size. In my case I get a compiler error when adding it because it's already implemented by the u32 conversion code. I uncommented it in my patch. Do you think I can safely remove the uncommented part?

||=== scriptinterface, Release ===|
/home/yves/Projekte/0ad_original_trunk/source/scriptinterface/ScriptConversions.cpp|279|Fehler: Redefinition von »static jsval ScriptInterface::ToJSVal(JSContext*, const T&) [with T = unsigned int; jsval = long unsigned int; JSContext = JSContext]«|
/home/yves/Projekte/0ad_original_trunk/source/scriptinterface/ScriptConversions.cpp|270|Fehler: »static jsval ScriptInterface::ToJSVal(JSContext*, const T&) [with T = unsigned int; jsval = long unsigned int; JSContext = JSContext]« wurde bereits hier deklariert|
||=== Build finished: 2 errors, 0 warnings (1 minutes, 23 seconds) ===|
||=== Build finished: 2 errors, 0 warnings (1 minutes, 28 seconds) ===|

[historic_bruno]

I think if you remove the ToJSVal part, you should remove the matching FromJSVal? Any ideas why only ToJSVal causes the error? I tested with VC++ 2010 (32-bit build) and your patch worked fine as-is and with the To/From unsigned pair removed. Philip would know more about it.

[Yves]

Thanks for testing! I actually already removed the FromJSVal part and only uncommented the ToJSVal function. Now I've removed both of them since they aren't needed on 64bit Windows, 32bit Windows or 64bit Linux. In v1.1 of the patch I forgot to include the removal of JSInterface_Vector3D which is now in the patch again in V1.2. Now I've also tested on x64 and VS2010. There were some errors in this case which are now fixed (inside the #if ARCH_AMD64). In addition I've fixed a compile warning in this file which isn't directly related to my change. I will commit the patch tomorrow if no problems are found (I want to avoid having to catch up to other changes if I wait for the next weekend).

[leper]

Replying to Yves:

Now I've also tested on x64 and VS2010. There were some errors in this case which are now fixed (inside the #if ARCH_AMD64). In addition I've fixed a compile warning in this file which isn't directly related to my change.

So it only errored on Windows and VS? If yes then that preprocessor if should be something like MSC_VERSION && ARCH_AMD64.

[Yves]

Well the error I'm talking about was a variable name I forgot to adjust ("Storage" in ScriptInterface::FromJSVal<ssize_t>). The issue with unsigned being the same as "u32" is a different topic.

These are the comments I took from the original implementation.

// (s)size_t are considered to be identical to (unsigned) int by GCC and  
// their specializations would cause conflicts there. On x86_64 GCC, s/size_t  
// is equivalent to (unsigned) long, but the same solution applies; use the  
// long and unsigned long specializations instead of s/size_t.  

// for some reason, x64 MSC treats size_t as distinct from unsigned long: 

Do you mean I should replace the line " #if ARCH_AMD64 " with " #if MSC_VERSION && ARCH_AMD64 "? I currently don't know what configuration needs to be used for other compilers like ICC, but apparently it worked in the past with this approach and should also work now. I wouldn't change that without a clear reason.

[Yves]

Removed a logwarning statement that was left from a test

[leper]

The code block in the if is only compiled if not running GCC (or compatible) and on amd64, and as that special case is only for fixing x64 MSC replacing both #ifs with #if MSC_VERSION && ARCH_AMD64 makes more sense.

ICC is (or should be) compatible to GCC on *nix (which is also true for clang), while it is compatible to MSC on Windows (see lib/sysdep/compiler.h and some ICC docs). So I guess both ICC and MSC would issue an error/warning when compiling on Windows, so using MSC_VERSION && ARCH_AMD64 would be the right thing to do IMO.

[Yves]

In 14036:

Unify script conversions and remove JSInterface_Vector3D.

Because it was historically grown, we have some duplicated code for converting script types to native types.
This patch removes the file JSConversions.cpp and moves some code to ScriptConversions.cpp.
The places using JSConversions.cpp are changed to use the ScriptInterface's conversion functions in ScriptConversions.cpp.
I also removed JSInterface_Vector3D because it had additional requirements to the conversion code that no other code has and because it's currently not used. I think it doesn't make sense to maintain code just because it could possibly be used again later.

Closes #2213
Refs #1886