Bug 51861 - DataView.getInt8() returns unsigned value on ARM platform
Summary: DataView.getInt8() returns unsigned value on ARM platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-03 18:09 PST by Xianzhu Wang
Modified: 2011-01-04 12:44 PST (History)
4 users (show)

See Also:


Attachments
patch (6.31 KB, patch)
2011-01-03 18:30 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (14.23 KB, patch)
2011-01-04 02:05 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2011-01-03 18:09:35 PST
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.
Comment 1 Xianzhu Wang 2011-01-03 18:30:53 PST
Created attachment 77863 [details]
patch
Comment 2 Eric Seidel (no email) 2011-01-03 23:48:36 PST
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?
Comment 3 Xianzhu Wang 2011-01-04 02:05:55 PST
Created attachment 77871 [details]
patch v2
Comment 4 Xianzhu Wang 2011-01-04 02:06:46 PST
(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.
Comment 5 WebKit Commit Bot 2011-01-04 12:44:10 PST
Comment on attachment 77871 [details]
patch v2

Clearing flags on attachment: 77871

Committed r74994: <http://trac.webkit.org/changeset/74994>
Comment 6 WebKit Commit Bot 2011-01-04 12:44:15 PST
All reviewed patches have been landed.  Closing bug.