Bug 31040 - Use explicit parentheses to silence gcc 4.4 -Wparentheses warnings
Summary: Use explicit parentheses to silence gcc 4.4 -Wparentheses warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-02 19:53 PST by Laszlo Gombos
Modified: 2009-11-09 12:05 PST (History)
2 users (show)

See Also:


Attachments
1st try (17.51 KB, patch)
2009-11-02 20:02 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
2nd try (17.50 KB, patch)
2009-11-03 15:09 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-11-02 19:53:15 PST
GCC 4.4 -Wparentheses now warns about expressions such as (!x | y) and (!x & y). Using explicit parentheses, such as in ((!x) | y), silences this warning -- see the release notes: http://gcc.gnu.org/gcc-4.4/changes.html

As an example QtWebKit buildbot is using GCC 4.4 here are some of the warnings:

JavaScriptCore/interpreter/Interpreter.cpp:1491: warning: suggest parentheses around arithmetic in operand of |
JavaScriptCore/interpreter/Interpreter.cpp:1578: warning: suggest parentheses around arithmetic in operand of |
WebCore/editing/TextIterator.cpp:221: warning: suggest parentheses around && within ||
WebCore/editing/VisibleSelection.cpp:240: warning: suggest parentheses around && within ||
WebCore/html/HTMLLinkElement.cpp:191: warning: suggest parentheses around && within ||
WebCore/loader/RedirectScheduler.cpp:174: warning: suggest parentheses around && within ||
WebCore/rendering/RenderBlock.cpp:956: warning: suggest parentheses around && within ||
WebCore/rendering/RenderBox.cpp:1474: warning: suggest parentheses around && within ||
WebCore/rendering/RenderTextControlMultiLine.cpp:68: warning: suggest parentheses around && within ||
WebCore/rendering/style/RenderStyle.cpp:458: warning: suggest parentheses around && within ||
WebCore/platform/graphics/qt/FontCacheQt.cpp:126: warning: suggest parentheses around + or - inside shift
WebCore/platform/graphics/qt/FontCacheQt.cpp:127: warning: suggest parentheses around + or - inside shift
WebCore/platform/graphics/qt/FontCacheQt.cpp:128: warning: suggest parentheses around + or - inside shift
WebKit/qt/Api/qwebpage.cpp:1213: warning: suggest parentheses around && within ||
WebKit/qt/Api/qwebpage.cpp:1219: warning: suggest parentheses around && within ||
WebKit/qt/Api/qwebpage.cpp:1223: warning: suggest parentheses around && within ||
WebCore/svg/SVGAnimateElement.cpp:67: warning: suggest parentheses around && within ||
WebCore/svg/SVGAnimationElement.cpp:492: warning: suggest parentheses around && within ||
WebCore/svg/SVGPreserveAspectRatio.cpp:183: warning: suggest parentheses around && within ||
WebCore/loader/appcache/ApplicationCacheGroup.cpp:625: warning: suggest parentheses around && within ||
WebCore/dom/Document.cpp:2494: warning: suggest parentheses around && within ||
Comment 1 Laszlo Gombos 2009-11-02 20:02:50 PST
Created attachment 42356 [details]
1st try

Place the parentheses according to C++ precedence rules.
Comment 2 Eric Seidel (no email) 2009-11-03 12:18:27 PST
Comment on attachment 42356 [details]
1st try

Some of these make sense.  And some of them don't:
 1578         if (src1.isInt32() && src2.isInt32() && !((src1.asInt32()) | (src2.asInt32() & 0xc0000000))) // no overflow


Why are parens needed around (src1.asInt32()) ??
Comment 3 Laszlo Gombos 2009-11-03 15:09:18 PST
Created attachment 42425 [details]
2nd try

Remove the extra () from around (src1.asInt32()).

Eric, thanks for the review - I think you found one case where I put a meaningless extra () in. I hope the rest of the changes looks good.
Comment 4 Kenneth Rohde Christiansen 2009-11-09 07:55:56 PST
LGTM. Does our style guide have a say about this? If not, maybe we should add a rule after discussion on the ML?
Comment 5 Laszlo Gombos 2009-11-09 11:45:17 PST
(In reply to comment #4)
> LGTM. Does our style guide have a say about this? If not, maybe we should add a
> rule after discussion on the ML?

Kenneth, thanks for the review.

I think that for warnings that compilers can catch the style guide might be redundant. However we should have a common agreement on the warnings we care about as a community.
Comment 6 WebKit Commit Bot 2009-11-09 12:04:57 PST
Comment on attachment 42425 [details]
2nd try

Clearing flags on attachment: 42425

Committed r50675: <http://trac.webkit.org/changeset/50675>
Comment 7 WebKit Commit Bot 2009-11-09 12:05:00 PST
All reviewed patches have been landed.  Closing bug.