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

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.