Last Comment Bug 756996 - GCC warnings in gfx/2d
: GCC warnings in gfx/2d
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
-- normal (vote)
: mozilla16
Assigned To: Jacek Caban
: Milan Sreckovic [:milan]
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image 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 User image Jacek Caban 2012-05-31 07:11:41 PDT
I've moved them back and marked inline:

Thanks for the review.
Comment 4 User image 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:
Comment 5 User image :Ms2ger (⌚ UTC+1/+2) 2012-06-04 04:08:59 PDT

should just be


Comment 6 User image 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 User image Geoff Lankow (:darktrojan) 2012-06-05 05:57:46 PDT

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