Bug 51861 – DataView.getInt8() returns unsigned value on ARM platform

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Xianzhu Wang]

LayoutTests/fast/canvas/webgl/data-view-test.html fails on ARM platform:

FAIL view.getInt8(8) should be -128. Was 128.
FAIL view.getInt8(15) should be -1. Was 255.

ARM's 'char' is 'unsigned char' by default because it is more efficient on ARM. 'Man g++' says "a portable program should always use 'signed char' or 'unsigned char' when it depends on the signedness of an object." Here is the case.

[Xianzhu Wang]

Created attachment 77863 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 77863 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77863&action=review

> WebCore/bindings/js/JSDataViewCustom.cpp:139
> +        imp->setInt8(byteOffset, static_cast<signed char>(value), ec);

Does setInt8 take a signed char explictly?  it seems it should.  Does the compiler warn about this sign conversion?  Again, it seems it should.

> WebCore/html/canvas/DataView.h:59
> +    void setInt8(unsigned byteOffset, signed char value, ExceptionCode&);

Ah, I see you're making ti signed here.  don't we have a less-verbose type to use than "signed char"?  int8_t or similar?

[Xianzhu Wang]

Created attachment 77871 [details]
patch v2

[Xianzhu Wang]

(In reply to comment #2)
> (From update of attachment 77863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77863&action=review
> 
> > WebCore/bindings/js/JSDataViewCustom.cpp:139
> > +        imp->setInt8(byteOffset, static_cast<signed char>(value), ec);
> 
> Does setInt8 take a signed char explictly?  it seems it should.  Does the compiler warn about this sign conversion?  Again, it seems it should.
> 
> > WebCore/html/canvas/DataView.h:59
> > +    void setInt8(unsigned byteOffset, signed char value, ExceptionCode&);
> 
> Ah, I see you're making ti signed here.  don't we have a less-verbose type to use than "signed char"?  int8_t or similar?

Thanks for review.
Changed signed char to int8_t, and also types of other methods to more precise integer types.

[WebKit Commit Bot]

Comment on attachment 77871 [details]
patch v2

Clearing flags on attachment: 77871

Committed r74994: <http://trac.webkit.org/changeset/74994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.