Use MOZ_ASSERT where appropriate in WebGL code

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Posted patch patchSplinter Review
No description provided.
Attachment #814189 - Flags: review?(jgilbert)
Comment on attachment 814189 [details] [diff] [review]
patch

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

Nits. We should really resurrect NS_NOTREACHED for MOZ_*, too.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2155,5 @@
>                  } else if (shader->ShaderType() == LOCAL_GL_FRAGMENT_SHADER) {
>                      shaderTypeName = "fragment";
>                  } else {
>                      // should have been validated earlier
> +                    MOZ_ASSERT(false);

We should really have a MOZ_ASSERT_NOTREACHED or something.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +221,5 @@
>              return true;
>      }
>  
>      NS_NOTREACHED("Invalid WebGL texture format?");
> +    MOZ_ASSERT(false);

This should be redundant.

::: content/canvas/src/WebGLTexelConversions.cpp
@@ +173,1 @@
>                            mDstStride % sizeof(DstType) == 0,

Fix the indents here.
Attachment #814189 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Comment on attachment 814189 [details] [diff] [review]
> patch
> 
> Review of attachment 814189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nits. We should really resurrect NS_NOTREACHED for MOZ_*, too.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +2155,5 @@
> >                  } else if (shader->ShaderType() == LOCAL_GL_FRAGMENT_SHADER) {
> >                      shaderTypeName = "fragment";
> >                  } else {
> >                      // should have been validated earlier
> > +                    MOZ_ASSERT(false);
> 
> We should really have a MOZ_ASSERT_NOTREACHED or something.

There actually is MOZ_ASSUME_UNREACHABLE in mfbt/Assertions.h.

But it warns about how it's intentionally undefined behavior, therefore unsafe and only useful as an optimization in critical code:

 * MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that it can assume that
 * the macro call cannot be reached during execution.  This lets the compiler
 * generate better-optimized code under some circumstances, at the expense of
 * the program's behavior being undefined if control reaches the
 * MOZ_ASSUME_UNREACHABLE.
 *
 * In Gecko, you probably should not use this macro outside of performance- or
 * size-critical code, because it's unsafe.  If you don't care about code size
 * or performance, you should probably use MOZ_ASSERT or MOZ_CRASH.

So let's stick to MOZ_CRASH here.

> 
> ::: content/canvas/src/WebGLContextUtils.cpp
> @@ +221,5 @@
> >              return true;
> >      }
> >  
> >      NS_NOTREACHED("Invalid WebGL texture format?");
> > +    MOZ_ASSERT(false);
> 
> This should be redundant.

Good catch, we only want the MOZ_ASSERT(false) here for the reason given above.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1632bbff03ad
Assignee: nobody → bjacob
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/1632bbff03ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.