WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93370
reimplement fastMod w/o (soon to be) private skia macros
https://bugs.webkit.org/show_bug.cgi?id=93370
Summary
reimplement fastMod w/o (soon to be) private skia macros
Mike Reed
Reported
2012-08-07 07:36:01 PDT
reimplement fastMod w/o (soon to be) private skia macros
Attachments
Patch
(1.66 KB, patch)
2012-08-07 07:37 PDT
,
Mike Reed
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Mike Reed
Comment 1
2012-08-07 07:37:55 PDT
Created
attachment 156939
[details]
Patch
Adrienne Walker
Comment 2
2012-08-07 11:25:07 PDT
Comment on
attachment 156939
[details]
Patch R=me. Those look functionally equivalent to me.
WebKit Review Bot
Comment 3
2012-08-07 11:45:03 PDT
Comment on
attachment 156939
[details]
Patch Clearing flags on attachment: 156939 Committed
r124901
: <
http://trac.webkit.org/changeset/124901
>
WebKit Review Bot
Comment 4
2012-08-07 11:45:07 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 5
2012-08-07 13:51:21 PDT
Comment on
attachment 156939
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=156939&action=review
> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:76 > + value = -value;
This won’t work if value is MIN_INT. I assume that’s OK since the old code already worked like that.
Allan Sandfeld Jensen
Comment 6
2012-08-07 14:58:50 PDT
Comment on
attachment 156939
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=156939&action=review
> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:72 > inline int fastMod(int value, int max)
Considering this a slow workaround for a something poorly defined in the C++ standard, I find the name fastMod a bit confusing. Have you considered a rename? I know this function is not from your patch, but since your patch makes it even slower it just makes the name weirder. Also do you actually have any examples of actual C++ implementations where (a % x) != -(-a % x). I thought this issue would be resolved by now. C++11 defined it to what all sane implementation already did, which is the same this function tries to return.
Mike Reed
Comment 7
2012-08-07 15:09:25 PDT
I think fastMod came from the >= check, so that we don't call % at all if value < max Good question if there are any real differences in how negatives are handled w.r.t. %. I haven't looked in a long while. Not sure the new one is slower actually. On architectures that have conditional instructions like ARM (and modern x86s), if (x < 0) x = -x; Can be two instructions w/ zero branches, as opposed to 2 or 3 for the extract/apply pattern. I haven't timed this particular use case tho. However, I agree that fastMod does not actually capture the whole intent: safeIntegerMod might be a better name.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug