866833 - GCC build warning: nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Daniel Holbert [:dholbert]]

The only two GCC build warnings in embedding/components/commandhandler are:
{
embedding/components/commandhandler/src/nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used
embedding/components/commandhandler/src/nsControllerCommandTable.cpp:184:12 [-Wunused-but-set-variable] variable 'rv' set but not used
}

These are for two instances of this pattern...
> 166   nsresult rv;
> 167   rv = FindCommandHandler(aCommandName, getter_AddRefs(commandHandler));
> 168   if (!commandHandler)
> 169   {

...where we capture 'rv' but we end up just checking whether the outparam is non-null as an indication of success.

This file has 4 other calls to FindCommandHandler that don't bother with the 'rv' charade and just use the null-check.  And from looking at the actual FindCommandHandler() impl in that file, this null-check seems to a fine indication of success/failure, in practice.

So, I think we should just drop the 'rv' charade here. (Alternately we could add a "if NS_FAILED(rv)" check, but the existing null-check already covers that, so it wouldn't really be useful.)

Comment 1

[Daniel Holbert [:dholbert]]

This affected the first version of this file, checked in in 2003, FWIW:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/commandhandler/src/nsControllerCommandTable.cpp&rev=1.1&mark=199-201,217-219#199

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: drop 'rv' since it's unnecessary and we don't bother with it elsewhere in this file

The file hasn't had any significant changes since then, so it's basically un-owned. Tagging bz for review, in his capacity as embedding peer.

(Note: The #if DEBUG in the patch-context is a bit silly; I'm ignoring it, because it's scattered elsewhere throughout the file, too, and it's harmless.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 743192 [details] [diff] [review]
fix v1: drop 'rv' since it's unnecessary and we don't bother with it elsewhere in this file

r=me

Comment 4

[Daniel Holbert [:dholbert]]

Thanks!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8a29cfe00a

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c8a29cfe00a