Now that ctypes is a spidermonkey citizen, sayrer's got his flaystick at the ready -- there are a bunch of warnings, different for each plat, that we should fix.

We can probably use std::numeric_limits to help here. http://msdn.microsoft.com/en-us/library/85084kd6.aspx

Please fix these today. We don't allow warnings in js/src anymore.

This fixes all the warnings in ctypes code... With the exception of three or so on Mac debug, where we're testing 'i < 0' where 'i' is a (templated) unsigned type. The only way to fix this to go mad with specialized templates. I have a patch, but I'm still tweaking it. There are some warnings in libffi, which I'll fix separately.

Comment on attachment 437142 [details] [diff] [review] fix warnings Wait, this is wrong.

This gets it right, and fixes all ctypes warnings. The templates are more than I'd like, but they do result in code simplification.

>-template<class IntegerType> >-static JS_ALWAYS_INLINE IntegerType >-Convert(jsdouble d) >-{ >- return IntegerType(d); >+JS_STATIC_ASSERT(sizeof(PRFuncPtr) == sizeof(void*)); >+JS_STATIC_ASSERT(numeric_limits<double>::is_signed); >+ >+template<class TargetType, class FromType> >+static JS_ALWAYS_INLINE TargetType >+Convert(FromType d) >+{ >+ return TargetType(d); > } > > #ifdef _MSC_VER > // MSVC can't perform double to unsigned __int64 conversion when the > // double is greater than 2^63 - 1. Help it along a little. > template<> > JS_ALWAYS_INLINE JSUint64 >-Convert<JSUint64>(jsdouble d) >+Convert<JSUint64, jsdouble>(jsdouble d) You can't partially specialize functions. This second declaration is really just overloading the Convert function. Unless I'm mistaken, you'll get a compiler error from MSVC if you call Convert<T>(d) where d is a jsdouble but T is not JSUint64. What you need is to define a struct type with an appropriate specialization, then implement Convert in terms of that: template<class TargetType, class FromType> struct Converter { static JS_ALWAYS_INLINE TargetType Convert(FromType d) { return TargetType(d); } }; #ifdef _MSC_VER // MSVC can't perform double to unsigned __int64 conversion when the // double is greater than 2^63 - 1. Help it along a little. template<> struct Converter<JSUint64, jsdouble> { static JS_ALWAYS_INLINE JSUint64 Convert(jsdouble d) { return d > 0x7fffffffffffffffui64 ? JSUint64(d - 0x8000000000000000ui64) + 0x8000000000000000ui64 : JSUint64(d); } }; #endif template<class TargetType, class FromType> static JS_ALWAYS_INLINE TargetType Convert(FromType d) { return Converter<TargetType, FromType>::Convert(d); }

Comment on attachment 437164 [details] [diff] [review] template madness >+template<class TargetType, class FromType> >+static JS_ALWAYS_INLINE bool IsAlwaysExact() >+{ >+ // Return 'true' if TargetType is always exactly representable by FromType. Don't you mean "if TargetType can always exactly represent FromType"?

(In reply to comment #6) > You can't partially specialize functions. This second declaration is really > just overloading the Convert function. Unless I'm mistaken, you'll get a > compiler error from MSVC if you call Convert<T>(d) where d is a jsdouble but T > is not JSUint64. Correct about partial specialization; but -- this is a full specialization, no? (Unless they're really the same thing.) It does pass tryserver, and Convert<> is certainly called with <T, double> where T != JSUint64. But maybe it's regardless asking for trouble and I should change it. (I've seen this partial specialization error from gcc before; I'll un-#ifdef the code and see what it thinks.) (In reply to comment #7) > Don't you mean "if TargetType can always exactly represent FromType"? Yes :)

Fixes, per comment.

For '"size" may be used uninitialized in this function'.

Comment on attachment 437425 [details] [diff] [review] v2 ConvertImpl is a fine name.

Comment on attachment 437445 [details] [diff] [review] bonus fix Righteous!

Comment on attachment 437425 [details] [diff] [review] v2 Both landed as http://hg.mozilla.org/tracemonkey/rev/d3ba080175a4. There is one remaining warning in ctypes code: 'CTypes.cpp:5600: warning: ‘i’ may be used uninitialized in this function'. This is wrong, and strikes me as unstable -- there are many such places in CTypes.cpp where we use an identical pattern. Thus fixing it consistently would degrade generated code quality. Rob, please advise on what you want here. There are some remaining warnings in libffi code. I'll do what I can to clean those up.

Fixes all the warnings I see on tryserver, adds them to libffi.patch, and as a bonus fixes the 'may be used uninitialized' warning in CTypes.cpp. (Whatever, I'll just do it for two sites.) I'll push the libffi fixes upstream.

Comment on attachment 438498 [details] [diff] [review] fix libffi warnings Looks good to me.

Comment on attachment 438498 [details] [diff] [review] fix libffi warnings http://hg.mozilla.org/tracemonkey/rev/a6606e924e1d

All warnings on tryserver plats are fixed.