nsGenericHTMLElement.cpp:1635:12: error: unused variable ‘rv’ [-Werror=unused-variable]

RESOLVED DUPLICATE of bug 717004

Status

()

Core
DOM: Core & HTML
RESOLVED DUPLICATE of bug 717004
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Bug 716338 turned on warnings-as-errors in content/html/content/src/Makefile.in

This breaks my local build (with --enable-warnings-as-errors), due to this build warning:
{
nsGenericHTMLElement.cpp:1635:12: error: unused variable ‘rv’ [-Werror=unused-variable]
}
(Assignee)

Comment 1

6 years ago
This is for this line:
> 1622 nsGenericHTMLElement::RestoreFormControlState(nsGenericHTMLElement* aContent,
> 1623                                               nsIFormControl* aControl)
> 1624 {
[...]
> 1635   rv = history->GetState(key, &state);
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp#1635
(Assignee)

Comment 2

6 years ago
The return value from this call has been ignored since this line of code was added, for bug 77834:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&file=nsGenericHTMLElement.cpp&rev2=1.343&rev1=1.342

We check whether |state| is null on the next line -- it looks like that's used as an indicator of whether the GetState() call failed.  (Hopefully that's a fair assumption?)
Blocks: 77834, 187528
(Assignee)

Comment 3

6 years ago
Actually, nsLayoutHistoryState::GetState only ever returns NS_OK.

That's the only impl of the nsILayoutHistoryState interface that I can find...
http://mxr.mozilla.org/mozilla-central/search?string=public+nsILayoutHistoryState&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
...so unless we expect extensions to provide additional impls for this interface, we can safely just assume that this method will always succeed.
(Assignee)

Comment 4

6 years ago
On the other hand, maybe it's better to be hygenic and check for success before dereferencing the potentially-uninitialized-value |state|.  (we actually know it will be initialized, but that's because we're looking across the interface boundary)
(Assignee)

Comment 5

6 years ago
Created attachment 587458 [details] [diff] [review]
fix: check rv before using returned-by-reference |state|
Attachment #587458 - Flags: review?(jst)
(Assignee)

Updated

6 years ago
Attachment #587458 - Attachment description: fix → fix: check rv before using returned-by-reference |state|
(Assignee)

Comment 6

6 years ago
I only just noticed this is an extension of bug 717004's issues. :)  (I was in "try to build; hit build error; file bug w/ patch; repeat " mode when I filed this.)

uping to that bug; posted a merged patch there.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 717004
(Assignee)

Updated

6 years ago
Attachment #587458 - Flags: review?(jst)
You need to log in before you can comment on or make changes to this bug.