Clean up DashPathEffect modulo math from r3761

Clean up DashPathEffect modulo math from r3761
Committed: https://code.google.com/p/skia/source/detail?r=3773

[epoger, 2012-04-25 21:06:11]

I think this rearrangement is more robust...

1. It checks for infinite phase or len BEFORE attempting to manipulate them
2. It puts the finite-precision fix after all the modulo math, in case we modify
the modulo math later
3. It eliminates the goto

[epoger, 2012-04-25 21:36:40]

On 2012/04/25 21:06:11, epoger wrote:
> I think this rearrangement is more robust...
> 
> 1. It checks for infinite phase or len BEFORE attempting to manipulate them
> 2. It puts the finite-precision fix after all the modulo math, in case we
modify
> the modulo math later
> 3. It eliminates the goto

Mike has pointed out that the code would be slower with this CL, because it
would have to call fmodf(dividend, divisor) even if 0 <= dividend < divisor. 
Better to keep our own check in there!

So we're better off without this change.

(I still might want to move the IsFinite() checks higher, though.)

[epoger, 2012-04-25 21:53:57]

Mike, please look again as of patchset 4 (no rush, though.)  Only
non-documentation change is moving the IsFinite() checks higher (and getting rid
of goto)

[reed1, 2012-04-26 00:54:28]

https://codereview.appspot.com/6124048/diff/3003/src/effects/SkDashPathEffect...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.appspot.com/6124048/diff/3003/src/effects/SkDashPathEffect...
src/effects/SkDashPathEffect.cpp:49: // Adjust phase to be between 0 and len,
"flipping" phase if negative.
Like examples, not sure that 20% is meaningful, since we don't treat phase as a
fraction of length, but as a flat value just like length.

[epoger, 2012-04-26 14:22:20]

https://codereview.appspot.com/6124048/diff/3003/src/effects/SkDashPathEffect...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.appspot.com/6124048/diff/3003/src/effects/SkDashPathEffect...
src/effects/SkDashPathEffect.cpp:49: // Adjust phase to be between 0 and len,
"flipping" phase if negative.
On 2012/04/26 00:54:28, reed1 wrote:
> Like examples, not sure that 20% is meaningful, since we don't treat phase as
a
> fraction of length, but as a flat value just like length.

Done.

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
File include/effects/SkDashPathEffect.h (right):

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
include/effects/SkDashPathEffect.h:21: /** intervals: array containing an even
number of entries (>=2), with
updated the header file documentation too.

[reed1, 2012-04-27 12:13:09]

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
File include/effects/SkDashPathEffect.h (right):

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
include/effects/SkDashPathEffect.h:39: Note: only affects framed paths.
s/framed/stroked

https://codereview.appspot.com/6124048/diff/10001/src/effects/SkDashPathEffec...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.appspot.com/6124048/diff/10001/src/effects/SkDashPathEffec...
src/effects/SkDashPathEffect.cpp:65: }
I think your earlier comment on my CL is correct: this should be

} else if (phase >= len) {

since we clearly don't need to perform that test again after we just asserted
that (phase <= len).

[epoger, 2012-04-27 13:21:56]

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
File include/effects/SkDashPathEffect.h (right):

https://codereview.appspot.com/6124048/diff/10001/include/effects/SkDashPathE...
include/effects/SkDashPathEffect.h:39: Note: only affects framed paths.
On 2012/04/27 12:13:10, reed1 wrote:
> s/framed/stroked

Done.

https://codereview.appspot.com/6124048/diff/10001/src/effects/SkDashPathEffec...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.appspot.com/6124048/diff/10001/src/effects/SkDashPathEffec...
src/effects/SkDashPathEffect.cpp:65: }
On 2012/04/27 12:13:10, reed1 wrote:
> I think your earlier comment on my CL is correct: this should be
> 
> } else if (phase >= len) {
> 
> since we clearly don't need to perform that test again after we just asserted
> that (phase <= len).

Done.

[reed1, 2012-04-27 13:34:02]

lgtm