nsCSSRendering.cpp:1077: warning: operation on 'temp' may be undefined

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
14 years ago
Last year

People

(Reporter: Biesinger, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning], )

Attachments

(1 attachment)

../../../../mozilla/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::DrawDashedSides(PRIntn, nsIRenderingContext&, const nsRect&, const nsStyleColor*, const nsStyleBorder*, const nsStyleOutline*, PRBool, const nsRect&, const nsRect&, PRIntn, nsRect*)':
../../../../mozilla/layout/base/nsCSSRendering.cpp:1004: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1011: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1070: warning: operation on 'temp' may be undefined
../../../../mozilla/layout/base/nsCSSRendering.cpp:1077: warning: operation on 'temp' may be undefined

And the code does do odd things:
1004             temp = temp-= (dashRect.height-adjust);

Either the "temp =" part or the "temp -=" part should probably be removed...
Whiteboard: [build_warning]
Assignee: nobody → atulagrwl
Status: NEW → ASSIGNED
Atul, is this the right patch?
Yes, this is the right patch. Actually now we have quite different warnings in the nsCSSRendering.cpp file. Current warnings are:

/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintOutline(nsPresContext*, nsRenderingContext&, nsIFrame*, const nsRect&, const nsRect&, nsStyleContext*)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:699: warning: 'width' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintBackgroundWithSC(nsPresContext*, nsRenderingContext&, nsIFrame*, const nsRect&, const nsRect&, nsStyleContext*, const nsStyleBorder&, PRUint32, nsRect*)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:1516: warning: 'color' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:1516: note: 'color' was declared here
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:2364: warning: 'isSolidBorder' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:2363: warning: 'currentBackgroundClip' may be used uninitialized in this function
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp: In static member function 'static void nsCSSRendering::PaintDecorationLine(gfxContext*, nscolor, const gfxPoint&, const gfxSize&, gfxFloat, gfxFloat, PRUint8, PRUint8, gfxFloat)':
/builds/slave/m-cen-osx64-ntly/build/layout/base/nsCSSRendering.cpp:3397: warning: 'oldLineWidth' may be used uninitialized in this function
nsFirstLetterFrame.cpp

Sorry, I forgot to mention those.
(In reply to Atul Aggarwal from comment #3)
> Yes, this is the right patch. Actually now we have quite different warnings
> in the nsCSSRendering.cpp file.


Then I suspect it probably should have been a different bug.

That said, I think these current warnings are mostly or entirely bogus, and the fixes make the code more confusing.
I completely agree that these are false warnings and make code tougher to read. But either there has a way to shut the gcc about false warnings or we have to fix them otherwise developers(including me) tends to overlook the warning if there are lots of them and serious bugs are missed. Otherwise there is no point of adding gcc flags in the builds for showing warnings which just fills build logs and make search difficult in case of any failure.
I think we should turn off these warnings instead.
Attachment #557620 - Flags: review?(dbaron)
Comment on attachment 557620 [details] [diff] [review]
Patch v1 to fix warning

I don't think initializing variables to nonsense values is a good idea.  It makes the code harder to understand.

We should depend on valgrind catching stuff like this, and I tend to think we should turn off this gcc warning if it's this noisy.
Attachment #557620 - Flags: review?(dbaron) → review-
I'd be more inclined to agree with you if it had caught things like <http://hg.mozilla.org/integration/mozilla-inbound/file/dd6e523b0b82/content/html/document/src/nsHTMLFragmentContentSink.cpp#l942> (which GCC did catch, but the warning was ignored because of the large number of unfixed warnings).
If you want to fix these warnings, the way to do it is to de-COM-taminate the methods and make them return things rather than take out parameters.
Comment on attachment 557620 [details] [diff] [review]
Patch v1 to fix warning

># HG changeset patch
># Parent 84233d215c4e8d2f2f71e3d36cb912f88d3c1498
># User Atul Aggarwal <atulagrwl@gmail.com>
># Date 1314906994 -19800
>
>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp
>+++ b/layout/base/nsCSSRendering.cpp
>@@ -691,17 +691,17 @@ nsCSSRendering::PaintOutline(nsPresConte
>                              const nsRect& aBorderArea,
>                              nsStyleContext* aStyleContext)
> {
>   nscoord             twipsRadii[8];
> 
>   // Get our style context's color struct.
>   const nsStyleOutline* ourOutline = aStyleContext->GetStyleOutline();
> 
>-  nscoord width;
>+  nscoord width = 0;
>   ourOutline->GetOutlineWidth(width);

David, are we actually guaranteed that ourOutline->mHasCachedOutline is true here?
(In reply to Ms2ger from comment #10)
> David, are we actually guaranteed that ourOutline->mHasCachedOutline is true
> here?

Yes, since it's always true.  nsRuleNode::ComputeOutlineData calls nsStyleOutline::RecalcData which sets it to true for every legal type of mOutlineWidth.  We should probably clean that up at some point.

(Then again, *this bug* is WORKSFORME.)
Suggesting closing the bug as David suggested.
Assignee: atulagrwl → nobody
Can this bug be closed per comment 12?
Flags: needinfo?(dbaron)
Yes.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(dbaron)
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.