Last Comment Bug 756996 - GCC warnings in gfx/2d
: GCC warnings in gfx/2d
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Jacek Caban
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-21 03:02 PDT by Jacek Caban
Modified: 2012-06-05 05:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (14.91 KB, patch)
2012-05-21 03:02 PDT, Jacek Caban
bas: review+
Details | Diff | Review

Description Jacek Caban 2012-05-21 03:02:30 PDT
Created attachment 625590 [details] [diff] [review]
fix v1.0

Unhandled enum value in switch statement is esp very noisy there. I've added defaults there, but probably in some cases default could be marked as unreachable. Other warnings were about static function was defained but not used (that's because they were in a headers and were not used by some header users; the fix requires marking them inline or moving to .cpp files). I've fixed also sign-compare warnings that seem safe in this case. The most useful seem to be unused variable warnings.
Comment 1 Bas Schouten (:bas.schouten) 2012-05-30 23:27:13 PDT
Comment on attachment 625590 [details] [diff] [review]
fix v1.0

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

Looks fine mostly.

::: gfx/2d/DrawTargetD2D.cpp
@@ +87,5 @@
>  
>  ID2D1Factory *DrawTargetD2D::mFactory;
>  IDWriteFactory *DrawTargetD2D::mDWriteFactory;
>  
> +static bool IsPatternSupportedByD2D(const Pattern &aPattern)

Let's make these inline and -not- move them into cpp files. I don't want to move them again when other files need this in the future.
Comment 2 Jacek Caban 2012-05-31 07:11:41 PDT
I've moved them back and marked inline:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3fe2aff1f5

Thanks for the review.
Comment 4 Jacek Caban 2012-06-04 04:07:00 PDT
The failure was because MOZ_NOT_REACHED_MARKER is not present at all for GCC older than 4.5 in Assertion.h. I believe it's a bug (it should be at least defined as empty), but that's a separated issue. I've removed that part from my patch it builds on try:
https://tbpl.mozilla.org/?tree=Try&rev=1b14871c3acf

https://hg.mozilla.org/integration/mozilla-inbound/rev/50fa36a821fd
Comment 5 :Ms2ger 2012-06-04 04:08:59 PDT
  MOZ_ASSERT(false);
  MOZ_NOT_REACHED_MARKER();

should just be

  MOZ_NOT_REACHED("Why");

really
Comment 6 Jacek Caban 2012-06-04 04:13:48 PDT
That would use MOZ_Assert instead of its macro version MOZ_ASSERT, which AFAIR is not desired in this code.
Comment 7 Geoff Lankow (:darktrojan) 2012-06-05 05:57:46 PDT
https://hg.mozilla.org/mozilla-central/rev/50fa36a821fd

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