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

RESOLVED FIXED in Firefox 51

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (obsolete)
(Assignee)

Comment 3

a year ago
Bug 1297276 will rename the unused.h to Unused.h, so set the dependency.
Depends on: 1297276
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
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

a year ago
mozreview-review
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.
Attachment #8783831 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 7

a year ago
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.
Flags: needinfo?(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)
Flags: needinfo?(jgilbert)
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 hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
(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.
Keywords: checkin-needed

Comment 12

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f91e6ea2609b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.