Bug 38158 - Fix gcc compiler warnings in chromium platform graphics code
Summary: Fix gcc compiler warnings in chromium platform graphics code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 17:52 PDT by James Robinson
Modified: 2010-05-06 18:03 PDT (History)
6 users (show)

See Also:

Attachments
Patch (10.11 KB, patch)
2010-04-26 18:06 PDT, James Robinson 		no flags Details | Formatted Diff | Diff
patch that will pass checkstyle (10.10 KB, patch)
2010-04-26 18:07 PDT, James Robinson 		no flags Details | Formatted Diff | Diff
Patch (11.40 KB, patch)
2010-05-04 17:03 PDT, James Robinson 		no flags Details | Formatted Diff | Diff
Patch (10.34 KB, patch)
2010-05-06 15:40 PDT, James Robinson 		eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-04-26 17:52:16 PDT 
Fix gcc compiler warnings in chromium platform graphics code
Comment 1 James Robinson 2010-04-26 18:06:03 PDT 
Created attachment 54360 [details]
Patch
Comment 2 James Robinson 2010-04-26 18:07:42 PDT 
Created attachment 54361 [details]
patch that will pass checkstyle
Comment 3 James Robinson 2010-04-26 18:08:21 PDT 
The warnings are from gcc 4.4.1-4ubuntu9, which is what is currently shipping on Karmic, with -Wall -Werror.
Comment 4 James Robinson 2010-04-27 14:52:27 PDT 
Comment on attachment 54361 [details]
patch that will pass checkstyle

Clearing flags on attachment: 54361

Committed r58340: <http://trac.webkit.org/changeset/58340>
Comment 5 James Robinson 2010-04-27 14:52:32 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 James Robinson 2010-04-27 14:52:54 PDT 
Thanks for the review!
Comment 7 James Robinson 2010-05-03 18:19:08 PDT 
Reverted: http://trac.webkit.org/changeset/58404
Comment 8 James Robinson 2010-05-04 17:02:11 PDT 
This was reverted because this snippet:
  if (x < walker.width())
in FontLinux.cpp would evaluate to false if 'x' was less than 'walker.width()' or if 'x' was negative, because walker.width() returned an unsigned value.  C's type promotion rules would promote a negative 'x' to a large positive integer.  I've changed walker.width() to return a signed value and changed this statement to:
  if (x >= 0 && x < walker.width())
Comment 9 James Robinson 2010-05-04 17:03:04 PDT 
Created attachment 55074 [details]
Patch
Comment 10 Eric Seidel (no email) 2010-05-05 22:15:58 PDT 
Comment on attachment 55074 [details]
Patch

Again, removing the unused locals, and replacing NULL/0 and fixing parens (all completely uncontrovercial, mindless changes) would be easier to do first.

The signed/unsigned stuff requires some thinking to review correctly. :(
Comment 11 James Robinson 2010-05-06 15:40:10 PDT 
Created attachment 55311 [details]
Patch
Comment 12 Eric Seidel (no email) 2010-05-06 15:45:17 PDT 
Comment on attachment 55311 [details]
Patch

 +      if (!((format == GraphicsContext3D::RGBA && type == GraphicsContext3D::UNSIGNED_BYTE) || (format == m_implementationColorReadFormat && type == m_implementationColorReadType))) {
As far as I can tell this is correct.  Certainly reads nicer.  I would probably have even used local variables if I had written this originally. :)  GCC warnings FTW.

WebCore/platform/graphics/chromium/FontLinux.cpp:608
 +          if (x >= 0 && x < walker.width()) {
I'm curious how, if at all, this changes behavior.  Is this testable?

Please add at least a ChangeLog comment explaining why the x >= does not change behavior and does not require testing.  Otherwise this looks good.
Comment 13 James Robinson 2010-05-06 16:18:59 PDT 
Committed r58916: <http://trac.webkit.org/changeset/58916>
Comment 14 James Robinson 2010-05-06 16:19:33 PDT 
Thanks for the review.  Landed with an explanation of the x >= 0 change in the ChangeLog and a citation of the relevant test.