Bug 93370 – reimplement fastMod w/o (soon to be) private skia macros

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Reed]

reimplement fastMod w/o (soon to be) private skia macros

[Mike Reed]

Created attachment 156939 [details]
Patch

[Adrienne Walker]

Comment on attachment 156939 [details]
Patch

R=me.  Those look functionally equivalent to me.

[WebKit Review Bot]

Comment on attachment 156939 [details]
Patch

Clearing flags on attachment: 156939

Committed r124901: <http://trac.webkit.org/changeset/124901>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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 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]

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.