Closed
Bug 1297315
Opened 8 years ago
Closed 8 years ago
Fix Wmaybe-uninitialized warning and get rid of goto statement in GLContextProviderGLX.cpp and GfxTexturesReporter.cpp
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: JamesCheng, Assigned: JamesCheng)
References
Details
Attachments
(1 file)
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•8 years 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•8 years 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•8 years 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•8 years 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)
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f91e6ea2609b
Status: NEW → RESOLVED
Closed: 8 years 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.
Description
•