830439 - nsWebBrowserFind.cpp:501:14: warning: variable ‘rv’ set but not used [-Wunused-but-set-variable]

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning:
{
embedding/components/find/src/nsWebBrowserFind.cpp: In member function ‘nsresult nsWebBrowserFind::GetSearchLimits(nsIDOMRange*, nsIDOMRange*, nsIDOMRange*, nsIDOMDocument*, nsISelection*, bool)’:

embedding/components/find/src/nsWebBrowserFind.cpp: variable ‘rv’ set but not used

embedding/components/find/src/nsWebBrowserFind.cpp:501:14: warning: variable ‘rv’ set but not used [-Wunused-but-set-variable]
}

Comment 1

[Daniel Holbert [:dholbert]]

This issue was present in the initial checkin of this code (nsWebBrowserFind::GetSearchLimits), in bug 123087:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/embedding/components/find/src&command=DIFF_FRAMESET&file=nsWebBrowserFind.cpp&rev2=1.11&rev1=1.10

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch adds NS_ENSURE_SUCCESS(rv,rv) for the two places where we currently just ignore |rv|.

The first method (GetRangeCount) shouldn't ever fail, because it only has 1 impl, which always returns NS_OK:
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#4646

The second method (GetRootNode) can fail, but we already check it for failure implicitly via NS_ENSURE_ARG_POINTER() on something derived from its outparam.  The NS_ENSURE_SUCCESS in this patch just makes us return slightly earlier, if it does fail (and be more hygenic about not touching outparams from a failed function-call).

Patch marks this directory as warning-free, too.  It's warning-free on my local machine, at least -- seeing how that holds up on other platforms:
  https://tbpl.mozilla.org/?tree=Try&rev=a085e9116975

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1a (no FAIL_ON_WARNINGS)

That TBPL run was red on Android & Mac due to other warnings (with the Mac warnings being nontrivial to fix -- something about deprecated functions), so I'm dropping the FAIL_ON_WARNINGS annotation.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 702014 [details] [diff] [review]
fix v1a (no FAIL_ON_WARNINGS)

r=mats without the FAIL_ON_WARNINGS (you probably uploaded the old patch)

Comment 5

[Daniel Holbert [:dholbert]]

Yup, I must've forgotten to qref or something.  Sorry about that, & thanks for the review!
 https://hg.mozilla.org/integration/mozilla-inbound/rev/eb448d8799da

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb448d8799da

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> That TBPL run was red on Android & Mac due to other warnings (with the Mac
> warnings being nontrivial to fix -- something about deprecated functions),
> so I'm dropping the FAIL_ON_WARNINGS annotation.

(Note: we no let deprecated-function build warnings run afoul of FAIL_ON_WARNINGS, and I fixed the other build warning, so we can annotate this directory now. I filed bug 837903 on that.)