Last Comment Bug 744543 - LayerManagerOGL doesn't build on Linux/clang
: LayerManagerOGL doesn't build on Linux/clang
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla14
Assigned To: George Wright (:gw280) (:gwright)
:
Mentors:
Depends on:
Blocks: clang
  Show dependency treegraph
 
Reported: 2012-04-11 12:37 PDT by George Wright (:gw280) (:gwright)
Modified: 2012-04-12 10:26 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 744543 - LayerManagerOGL doesn't build on Linux/clang. r=? (1.07 KB, patch)
2012-04-11 12:38 PDT, George Wright (:gw280) (:gwright)
jacob.benoit.1: review+
Details | Diff | Review

Description George Wright (:gw280) (:gwright) 2012-04-11 12:37:43 PDT
We get this compile error:

/home/george/dev/mozilla-central/gfx/layers/opengl/LayerManagerOGL.cpp:265:7: error:
  non-constant-expression cannot be narrowed from type 'int' to 'GLenum' (aka 'unsigned
  int') in initializer list mGLContext->IsGLES2() ? LOCAL_GL_TEXTURE_RECTANGLE_ARB : 0
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 1 George Wright (:gw280) (:gwright) 2012-04-11 12:38:51 PDT
Created attachment 614115 [details] [diff] [review]
Bug 744543 - LayerManagerOGL doesn't build on Linux/clang. r=?
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-04-11 12:43:45 PDT
Comment on attachment 614115 [details] [diff] [review]
Bug 744543 - LayerManagerOGL doesn't build on Linux/clang. r=?

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

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +266,5 @@
>      };
>  
> +    if (mGLContext->IsGLES2()) {
> +        textureTargets[1] = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> +    }

No need for braces, per coding style. They uselessly reduce risk of future bugs, which is an unfair advantage given to future developers over us.
Comment 3 :Ms2ger 2012-04-11 12:46:15 PDT
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 614115 [details] [diff] [review]
> Bug 744543 - LayerManagerOGL doesn't build on Linux/clang. r=?
> 
> Review of attachment 614115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +266,5 @@
> >      };
> >  
> > +    if (mGLContext->IsGLES2()) {
> > +        textureTargets[1] = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> > +    }
> 
> No need for braces, per coding style. They uselessly reduce risk of future
> bugs, which is an unfair advantage given to future developers over us.

The coding style disagrees, and wants braces.
Comment 4 George Wright (:gw280) (:gwright) 2012-04-11 12:51:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd071b2e8a16
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-04-11 12:57:59 PDT
(In reply to Ms2ger from comment #3)
> The coding style disagrees, and wants braces.

Indeed. But on the other hand, the MFBT coding style (mfbt/STYLE) doesn't want braces here. We need to harmonize this a bit...
Comment 6 Jeff Gilbert [:jgilbert] 2012-04-11 16:17:59 PDT
Comment on attachment 614115 [details] [diff] [review]
Bug 744543 - LayerManagerOGL doesn't build on Linux/clang. r=?

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

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +266,4 @@
>      };
>  
> +    if (mGLContext->IsGLES2()) {
> +        textureTargets[1] = LOCAL_GL_TEXTURE_RECTANGLE_ARB;

4-space indent in 2-space land.

@@ +273,5 @@
>  
>      for (PRUint32 i = 0; i < ArrayLength(textureTargets); i++) {
>        GLenum target = textureTargets[i];
>        if (!target)
>            continue;

If you don't mind fixing it, this should also be 2-space.
Comment 7 Jeff Gilbert [:jgilbert] 2012-04-11 16:18:39 PDT
Alas, I am too late, as you already hit inbound with it.
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-12 10:26:11 PDT
https://hg.mozilla.org/mozilla-central/rev/bd071b2e8a16

Note You need to log in before you can comment on or make changes to this bug.