556902 - Fix ctypes warnings

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

We can probably use std::numeric_limits to help here.

http://msdn.microsoft.com/en-us/library/85084kd6.aspx

Comment 2

[Robert Sayre]

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

Comment 3

[[e-mail]]

Attachment: fix warnings

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 4

[[e-mail]]

Comment on attachment 437142 [details] [diff] [review]
fix warnings

Wait, this is wrong.

Comment 5

[[e-mail]]

Attachment: template madness

This gets it right, and fixes all ctypes warnings.

The templates are more than I'd like, but they do result in code simplification.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

>-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 7

[Ben Newman (:bnewman) (:benjamn)]

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"?

Comment 8

[[e-mail]]

(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 :)

Comment 9

[[e-mail]]

Attachment: v2

Fixes, per comment.

Comment 10

[[e-mail]]

Attachment: bonus fix

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

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 437425 [details] [diff] [review]
v2

ConvertImpl is a fine name.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

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

Righteous!

Comment 13

[[e-mail]]

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.

Comment 14

[[e-mail]]

Attachment: fix libffi warnings

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 15

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 438498 [details] [diff] [review]
fix libffi warnings

Looks good to me.

Comment 16

[[e-mail]]

Comment on attachment 438498 [details] [diff] [review]
fix libffi warnings

http://hg.mozilla.org/tracemonkey/rev/a6606e924e1d

Comment 17

[[e-mail]]

All warnings on tryserver plats are fixed.