Make CheckedInt<long> and CheckedInt<unsigned long> work

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

Currently CheckedInt<int>, CheckedInt<long long> works, but not CheckedInt<long>.

From compiling error, seems like the code was confused on what's the next level of integer above long.

That should be platform dependent.  If long is 32bit, then the next level should be long long.  If long is 64 bit, then the  next level should be undefined.

Fixing this could simply be a check of the sizeof(long) and map it either to int or long long.

Once this is fixed, we should use GC3D types when using CheckedInt instead of int32_t and uint32_t we currently use.

[Kenneth Russell]

CC'd Benoit, who wrote the CheckedInt class and who may have some suggestions on the right way to handle this issue.

[Benoit Jacob]

Could you please give me again the link to your copy of CheckedInt.h so I can see why CheckedInt<long> doesn't work for you?

[Kenneth Russell]

http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CheckedInt.h

To be clear, we have typedefs which match OpenGL's ("GC3Dfloat", "GC3Dint", etc.) and would like to be able to use those as arguments to CheckedInt, in particular "GC3Dintptr" which will be a 64-bit signed integer type on 64-bit platforms.

Thanks for looking into this.

[Benoit Jacob]

OK, so, at a quick glance, the most likely reason for this not working is that CheckedInt relies on template specializations of integer_type_manually_recorded_info<T> for types such as int32_t, int64_t, etc, and probably 'long' doesn't match any of those types, hence it fails to compile?

One must be careful here of not breaking the case where long is the same type as one of the int32_t (etc) types. The solution is to let the general case of integer_type_manually_recorded_info<T> check for a few types such as long, int, etc, before bailing out and declaring the type as unsupported as it currently does. Will provide patch asap.

[Zhenyao Mo]

(In reply to comment #4)
> OK, so, at a quick glance, the most likely reason for this not working is that CheckedInt relies on template specializations of integer_type_manually_recorded_info<T> for types such as int32_t, int64_t, etc, and probably 'long' doesn't match any of those types, hence it fails to compile?
> 
> One must be careful here of not breaking the case where long is the same type as one of the int32_t (etc) types. The solution is to let the general case of integer_type_manually_recorded_info<T> check for a few types such as long, int, etc, before bailing out and declaring the type as unsupported as it currently does. Will provide patch asap.

Thanks for looking into this!

The compiler is complaining about <T=long, U=long> not supported.
If I put int, it is <T=int32_t, U=int64_t>, and if I put long long, it is <T=int64_6, U=undefined>, and both compiles fine.

[Benoit Jacob]

OK, so that confirms exactly the hypothesis of comment 4.

[Benoit Jacob]

I have made a patch against our version, see the last patch at:
https://bugzilla.mozilla.org/show_bug.cgi?id=555798

"fix CheckedInt compilation for standard types"
https://bug555798.bugzilla.mozilla.org/attachment.cgi?id=503866

[Zhenyao Mo]

(In reply to comment #7)
> I have made a patch against our version, see the last patch at:
> https://bugzilla.mozilla.org/show_bug.cgi?id=555798
> 
> "fix CheckedInt compilation for standard types"
> https://bug555798.bugzilla.mozilla.org/attachment.cgi?id=503866

Thanks a lot!

[Benoit Jacob]

Updated, please use this newer patch:
https://bugzilla.mozilla.org/attachment.cgi?id=503872

[Kenneth Russell]

Mo, could you test and (unofficially) review this patch?

[Zhenyao Mo]

(In reply to comment #10)
> Mo, could you test and (unofficially) review this patch?

Sure.

[Zhenyao Mo]

(In reply to comment #9)
> Updated, please use this newer patch:
> https://bugzilla.mozilla.org/attachment.cgi?id=503872

Patch looks great!  I am merging this into webkit.

[Zhenyao Mo]

Created attachment 79328 [details]
Patch

[WebKit Review Bot]

Attachment 79328 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/html/canvas/CheckedInt.h:63:  integer_type_manually_recorded_info is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/html/canvas/CheckedInt.h:66:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/html/canvas/CheckedInt.h:71:  unsigned_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/html/canvas/CheckedInt.h:89:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:92:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:94:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:96:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:98:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:100:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:102:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:104:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:106:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:108:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:110:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:116:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:118:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:120:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:122:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:124:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:126:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:128:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:130:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/html/canvas/CheckedInt.h:133:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 23 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 79328 [details]
Patch

We should consider moving the CheckedInt header to a directory containing third-party code so that check-webkit-style can avoid producing false positives. See https://bugs.webkit.org/show_bug.cgi?id=52636 . Recent discussion on webkit-dev indicated that the Source/WebCore/thirdparty directory should be moved out of WebCore, though. I'm r+'ing this despite the style failures because we need to pick up this fix.

[Zhenyao Mo]

Committed r76067: <http://trac.webkit.org/changeset/76067>