869188 - Compile error in Blur.cpp: "non-type template argument of type 'bool' is not an integral constant expression"

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Drew Willcoxon :adw]

Building a recent mozilla-inbound debug on OS X 10.7 with clang version 3.1 (tags/Apple/clang-318.0.61) (based on LLVM 3.1svn):

> In file included from /Users/adw/mi/gfx/2d/Blur.cpp:7:
> In file included from ../../dist/include/mozilla/gfx/Blur.h:12:
> ../../dist/include/mozilla/CheckedInt.h:179:31: error: non-type template argument of type 'bool' is not an integral constant expression
>     static const bool value = IntegerType(-1) <= IntegerType(0);
>                               ^
> ../../dist/include/mozilla/CheckedInt.h:343:28: note: while checking a default template argument used here
>   return IsInRangeImpl<T, U>::run(x);
>          ~~~~~~~~~~~~~~~~~~^
> ../../dist/include/mozilla/CheckedInt.h:578:18: note: in instantiation of function template specialization 'mozilla::detail::IsInRange<int, float>' requested here
>         mIsValid(detail::IsInRange<T>(value))
>                  ^
> /Users/adw/mi/gfx/2d/Blur.cpp:404:37: note: in instantiation of function template specialization 'mozilla::CheckedInt<int>::CheckedInt<float>' requested here
>   CheckedInt<int32_t> minDataSize = CheckedInt<int32_t>(aRect.width)*aRect.height;
>                                     ^

bjacob says this:

> bjacob: adw: we are doing something weird here, that we shouldn't: initialize a CheckedInt<int32> from a float value
> bjacob: adw: we need to fix the Blur.cpp code, and make CheckedInt consistently reject this
> bjacob: adw: meanwhile, if you just want a build,
> bjacob: adw: add a conversion to int32_t as follows:    = CheckedInt<int32_t>(int32_t(aRect.width))*aRect.height;
> bjacob: oh and you need to cast height too

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Wow: this file is full of CheckedInts constructed from floats and/or multiplied by floats. We need to fix that; understand how this could ever compile; and make sure that it doesn't compile ever again.

Self assigning.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Cast aRect to an IntRect

Is this the right way?

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 746104 [details] [diff] [review]
Cast aRect to an IntRect

Review of attachment 746104 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Blur.cpp
@@ +401,5 @@
>      mStride(aStride),
>      mSurfaceAllocationSize(-1)
>  {
> +  IntRect intRect;
> +  if (aRect.ToIntRect(&intRect)) {

Maybe we should add an assertion if this doesn't succeed? I'm not sure why it wouldn't, but better safe than sorry.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Your call. I just followed what this function was doing in the case where the CheckedInt was invalid.

One thing to keep in mind though: you don't want to assert on something that can be triggered by content.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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