1040582 - Expose nsLayoutUtils.cpp's MULDIV() somewhere public

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

nsLayoutUtils.cpp has:
> #define MULDIV(a,b,c) (nscoord(int64_t(a) * int64_t(b) / int64_t(c)))

I needed it in flexbox code (for bug 1015474), so I'm copypasting the macro for now (since it's just a one-liner), but we should really find a better place for this to live.

Mats suggests moving it to nscoord.h in bug 1015474 comment 11.  Seems reasonable, since it's an operation on nscoords and it only involves nscoord and int64_t.  (We should perhaps make it an actual inline function, instead of a macro, for better type-safety, too.)

Comment 1

[Arnaud Sourioux [:Six]]

Attachment: expose_muldiv_macro_as_inline_function.patch

Is this what is expected?

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8480879 [details] [diff] [review]
expose_muldiv_macro_as_inline_function.patch

Thanks for picking this up!

(In reply to Arnaud Sourioux [:Six] from comment #1)
> Is this what is expected?
>+inline nscoord MULDIV(int64_t a, int64_t b, int64_t c) {
>+  return (a * b / c);

Almost.

Nits:
 * The args a, b, and c should have type "nscoord", not int64_t, and we can do the cast internally (like we do in the original macro).  The idea is, "multiply these two nscoord values, and divide by a third".  The int64_t cast is an implementation detail, which shouldn't be exposed through the function signature.
 * We should give them args descriptive names, maybe "aMult1, aMult2, aDiv"
 * We should name it something like NSCoordMulDiv(), to match the other utility methods in nscoord.h
 * The implementation should have an "#ifdef NS_COORD_IS_FLOAT" guard, and simply do the multiplication/division (no int64_t casting necessary) in that clause, and do the int64_t-casting-version in the #else clause

Comment 3

[Arnaud Sourioux [:Six]]

Attachment: expose_muldiv_macro_as_inline_function.patch

This version includes your last comment
Thanks for the details

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8480903 [details] [diff] [review]
expose_muldiv_macro_as_inline_function.patch

Thanks! r=me with two nits fixed:

># HG changeset patch
># Parent d697d649c76557865c96636f4c2ca805a77acbaa
># User Arnaud Sourioux <[e-mail]>
>Bug 1040582: Change nsLayoutUtils.cpp's MULDIV Macro as inline function in nsCoord.h r=dholbert

s/as inline/to inline/

>diff --git a/gfx/src/nsCoord.h b/gfx/src/nsCoord.h
>--- a/gfx/src/nsCoord.h
>+++ b/gfx/src/nsCoord.h
>@@ -45,16 +45,24 @@ typedef float nscoord;
> #define nscoord_MAX NS_IEEEPositiveInfinity()
> #else
> typedef int32_t nscoord;
> #define nscoord_MAX nscoord(1 << 30)
> #endif
> 
> #define nscoord_MIN (-nscoord_MAX)
> 
>+inline nscoord NSCoordMulDiv(nscoord aMult1, nscoord aMult2, nscoord aDiv) {
>+#ifdef NS_COORD_IS_FLOAT
>+  return (aMult1 * aMult2 / aDiv);
>+#else
>+  return (int64_t(aMult1) * int64_t(aMult2) / int64_t(aDiv));
>+#endif
>+}
>+
> inline void VERIFY_COORD(nscoord aCoord) {
> #ifdef NS_COORD_IS_FLOAT
>   NS_ASSERTION(floorf(aCoord) == aCoord,
>                "Coords cannot have fractions");
> #endif
> }

Please move the new function below VERIFY_COORD, so that it's next to the other arithmetic functions.

(VERIFY_COORD seems kind of special and is intentionally separate from the arithmetic functions, so it's probably best to avoid sandwiching it between arithmetic functions by sticking something above it.)

Comment 5

[Arnaud Sourioux [:Six]]

Attachment: expose_muldiv_macro_as_inline_function.patch

New patch including last dholbert comment.

Comment 6

[Arnaud Sourioux [:Six]]

Tbpl looks green:
https://tbpl.mozilla.org/?tree=Try&rev=6c55d25513c3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfcd20daf182

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfcd20daf182