add round/ceil/etc. for SkMScalar

add round/ceil/etc. for SkMScalar

BUG=skia:
TBR=

Committed: https://skia.googlesource.com/skia/+/39393e3ac309d4bedbc18bae98d3ebcb762501dd

[reed1, 2014-10-21 18:50:50]

reed@google.com changed reviewers:
+ pkasting@chromium.org

[reed1, 2014-10-21 18:50:55]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-10-21 18:52:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645793006/20001

[commit-bot: I haz the power, 2014-10-21 18:52:02]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-10-22 00:52 UTC

[Peter Kasting, 2014-10-21 18:56:09]

Can we also add an SkIntToMScalar?

https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
File include/core/SkFloatingPoint.h (right):

https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
include/core/SkFloatingPoint.h:120: #define sk_double_round(x)         
floor((x) + 0.5)
This doesn't match std::round() for negative numbers (this rounds toward
positive infinity instead of rounding away from 0).  Do you care?

Same issue with sk_double_round2int.

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp
File tests/Matrix44Test.cpp (right):

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
tests/Matrix44Test.cpp:764: // just want to exercise the various converters for
MScalar
Nit: Capitalization, period

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
tests/Matrix44Test.cpp:774: SkMScalarCeilToInt(mat.get(2, 3));
Nit: This formatting is not really standard...

[reed1, 2014-10-21 19:08:01]

On 2014/10/21 18:56:09, Peter Kasting wrote:
> Can we also add an SkIntToMScalar?
> 
>
https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
> File include/core/SkFloatingPoint.h (right):
> 
>
https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
> include/core/SkFloatingPoint.h:120: #define sk_double_round(x)         
> floor((x) + 0.5)
> This doesn't match std::round() for negative numbers (this rounds toward
> positive infinity instead of rounding away from 0).  Do you care?
> 
> Same issue with sk_double_round2int.
> 
> https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp
> File tests/Matrix44Test.cpp (right):
> 
>
https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
> tests/Matrix44Test.cpp:764: // just want to exercise the various converters
for
> MScalar
> Nit: Capitalization, period
> 
>
https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
> tests/Matrix44Test.cpp:774: SkMScalarCeilToInt(mat.get(2, 3));
> Nit: This formatting is not really standard...

SkIntToMScalar -- yes, guess I should have waited for your replay *before*
writing a bunch of code :)

[reed1, 2014-10-21 19:08:08]

https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
File include/core/SkFloatingPoint.h (right):

https://codereview.chromium.org/645793006/diff/20001/include/core/SkFloatingP...
include/core/SkFloatingPoint.h:120: #define sk_double_round(x)         
floor((x) + 0.5)
On 2014/10/21 18:56:09, Peter Kasting wrote:
> This doesn't match std::round() for negative numbers (this rounds toward
> positive infinity instead of rounding away from 0).  Do you care?
> 
> Same issue with sk_double_round2int.

Agreed, this is very deliberate on skia's part. We want to round in a
"consistent" dirction, so that we are somewhat translation-independent.

[reed1, 2014-10-21 19:10:41]

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp
File tests/Matrix44Test.cpp (right):

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
tests/Matrix44Test.cpp:764: // just want to exercise the various converters for
MScalar
On 2014/10/21 18:56:09, Peter Kasting wrote:
> Nit: Capitalization, period

Acknowledged.

https://codereview.chromium.org/645793006/diff/20001/tests/Matrix44Test.cpp#n...
tests/Matrix44Test.cpp:774: SkMScalarCeilToInt(mat.get(2, 3));
On 2014/10/21 18:56:09, Peter Kasting wrote:
> Nit: This formatting is not really standard...

ack. shooting for readability that we covered the 6 variants.

[Peter Kasting, 2014-10-21 19:13:01]

https://codereview.chromium.org/645793006/diff/40001/include/utils/SkMatrix44.h
File include/utils/SkMatrix44.h (right):

https://codereview.chromium.org/645793006/diff/40001/include/utils/SkMatrix44...
include/utils/SkMatrix44.h:79: #define SkIntToMScalar(n)       ((SkMScalar)(n))
Why not static_cast instead of C-style casting?

Should we also include this in a test somewhere?

[reed1, 2014-10-21 19:17:58]

https://codereview.chromium.org/645793006/diff/40001/include/utils/SkMatrix44.h
File include/utils/SkMatrix44.h (right):

https://codereview.chromium.org/645793006/diff/40001/include/utils/SkMatrix44...
include/utils/SkMatrix44.h:79: #define SkIntToMScalar(n)       ((SkMScalar)(n))
On 2014/10/21 19:13:01, Peter Kasting wrote:
> Why not static_cast instead of C-style casting?
> 
> Should we also include this in a test somewhere?

Done.

[Peter Kasting, 2014-10-21 19:22:03]

LGTM

[reed1, 2014-10-21 19:23:21]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-10-21 19:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645793006/60001

[commit-bot: I haz the power, 2014-10-21 19:23:30]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-10-22 01:23 UTC

[reed1, 2014-10-21 19:32:58]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-10-21 19:33:02]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-10-21 19:33:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645793006/60001

[commit-bot: I haz the power, 2014-10-21 19:33:26]

Committed patchset #4 (id:60001) as 39393e3ac309d4bedbc18bae98d3ebcb762501dd