1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[James Cheng[:JamesCheng]]

Try Fix 
1. 
Warning: -Wmaybe-uninitialized in gfx/gl/GLContextProviderGLX.cpp: 'pixmap' may be used uninitialized in this function gfx/gl/GLContextProviderGLX.cpp:1328:80: warning: 'pixmap' may be used uninitialized in this function [-Wmaybe-uninitialized]

and 
gfx/gl/GfxTexturesReporter.cpp: 'unit' may be used uninitialized in this function
gfx/gl/GfxTexturesReporter.cpp:52:31: warning: 'unit' may be used uninitialized in this function [-Wmaybe-uninitialized]

2. Remove goto statement in C++ code.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp.

Review commit: https://reviewboard.mozilla.org/r/73496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73496/

Comment 2

[James Cheng[:JamesCheng]]

Hi jmuizelaar,

I don't know who did the magic that passed -Wno-error=maybe-uninitialized to g++...

So it is against the warning as error.

Also I think removing goto statement is benefit for C++ code.

Please help to review this patch for fixing those warnings.

Thank you!

Comment 3

[James Cheng[:JamesCheng]]

Bug 1297276 will rename the unused.h to Unused.h, so set the dependency.

Comment 4

[James Cheng[:JamesCheng]]

Comment on attachment 8783831 [details]
Bug 1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73496/diff/1-2/

Comment 5

[James Cheng[:JamesCheng]]

Hi jgilbert,

I don't know who did the magic that passed -Wno-error=maybe-uninitialized to g++...

So it is against the warning as error.

Also I think removing goto statement is benefit for C++ code.

Please help to review this patch for fixing those warnings.

Thank you!

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8783831 [details]
Bug 1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp.

https://reviewboard.mozilla.org/r/73496/#review71872

::: gfx/gl/GLContextProviderGLX.cpp:887
(Diff revision 2)
> -        glContext = nullptr; // note: this must be done while the graceful X error handler is set,
> +            glContext = nullptr; // note: this must be done while the graceful X error handler is set,
> -                            // because glxMakeCurrent can give a GLXBadDrawable error
> +                                // because glxMakeCurrent can give a GLXBadDrawable error
> -    }
> +        }
>  
> -    return glContext.forget();
> +        return glContext.forget();
> +    } while (true);

`while (false)`

::: gfx/gl/GLContextProviderGLX.cpp:1322
(Diff revision 2)
> -    if (error || serverError)
> +    if (error || serverError) {
>          return nullptr;
> +    }

This doesn't need {}s. Don't change it.

Comment 7

[James Cheng[:JamesCheng]]

Thanks for reviewing, please see my reply below.

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 8783831 [details]
> Bug 1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement
> in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp.
> 
> https://reviewboard.mozilla.org/r/73496/#review71872
> 
> ::: gfx/gl/GLContextProviderGLX.cpp:887
> (Diff revision 2)
> > -        glContext = nullptr; // note: this must be done while the graceful X error handler is set,
> > +            glContext = nullptr; // note: this must be done while the graceful X error handler is set,
> > -                            // because glxMakeCurrent can give a GLXBadDrawable error
> > +                                // because glxMakeCurrent can give a GLXBadDrawable error
> > -    }
> > +        }
> >  
> > -    return glContext.forget();
> > +        return glContext.forget();
> > +    } while (true);
> 
> `while (false)`
If I use |while (false)|, it will break the loop without continuing, the logic is not as expectation.


> 
> ::: gfx/gl/GLContextProviderGLX.cpp:1322
> (Diff revision 2)
> > -    if (error || serverError)
> > +    if (error || serverError) {
> >          return nullptr;
> > +    }
> 
> This doesn't need {}s. Don't change it.


Isn't it a good habit to use {} even it is only one line?
Or any policy that I should not modify this part.

Thank you.

Comment 8

[Kelsey Gilbert [:jgilbert]]

You're right about do{}while(false). We need while(true) for continue.

In this module, we want {} for single lines unless they are control flow statements. (return, break, continue)

Comment 9

[Jonathan Kew [:jfkthame]]

Mozilla coding style calls for bracing all control structures, even when the braces are redundant.[1] Is there a compelling reason for this code _not_ to follow that guideline?

(Skimming through the file concerned, I see that it's a bit of a mix, but even for the if-statements where the controlled code is a single |return| statement, the braced examples outnumber brace-less by at least 2:1, so it doesn't look like "use the prevailing style" readily trumps the style guide here...)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

Comment 10

[James Cheng[:JamesCheng]]

Comment on attachment 8783831 [details]
Bug 1297315 - Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73496/diff/2-3/

Comment 11

[James Cheng[:JamesCheng]]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> You're right about do{}while(false). We need while(true) for continue.
> 
> In this module, we want {} for single lines unless they are control flow
> statements. (return, break, continue)

Thank you for reviewing, 

I did not modify the original style keeping it one line.


But I thnink Jonathan's comment is worth adopting however.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f91e6ea2609b
Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp. r=jgilbert

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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