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

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: adw, Assigned: bjacob)

Tracking

Trunk
mozilla23
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
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.
Assignee: nobody → bjacob
Is this the right way?
Attachment #746104 - Flags: review?(bas)
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.
Attachment #746104 - Flags: review?(bas) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/a6934b3ae7ca
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 869194
You need to log in before you can comment on or make changes to this bug.