Closed Bug 1297315 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

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.
Bug 1297276 will rename the unused.h to Unused.h, so set the dependency.
Depends on: 1297276
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 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+
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
(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
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
https://hg.mozilla.org/mozilla-central/rev/f91e6ea2609b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.