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

Status: RESOLVED DUPLICATE of bug 717004

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

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]
}

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

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?)

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix: check rv before using returned-by-reference |state|

Comment 6

[Daniel Holbert [:dholbert]]

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.