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.
CC'd Benoit, who wrote the CheckedInt class and who may have some suggestions on the right way to handle this issue.
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?
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.
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.
(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.
OK, so that confirms exactly the hypothesis of comment 4.
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
(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!
Updated, please use this newer patch: https://bugzilla.mozilla.org/attachment.cgi?id=503872
Mo, could you test and (unofficially) review this patch?
(In reply to comment #10) > Mo, could you test and (unofficially) review this patch? Sure.
(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.
Created attachment 79328 [details] Patch
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.
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.
Committed r76067: <http://trac.webkit.org/changeset/76067>