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

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.)
(Assignee)

Updated

5 years ago
Summary: /nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used → nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used
(Assignee)

Comment 1

5 years ago
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
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 201257
(Assignee)

Comment 2

5 years ago
Created attachment 743192 [details] [diff] [review]
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.)
Attachment #743192 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #743192 - Attachment description: fix v1 → fix v1: drop 'rv' since it's unnecessary and we don't bother with it elsewhere in this file
(Assignee)

Updated

5 years ago
Summary: nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used → GCC build warning: nsControllerCommandTable.cpp:166:12 [-Wunused-but-set-variable] variable 'rv' set but not used
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
Attachment #743192 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

5 years ago
Thanks!
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 866983

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5c8a29cfe00a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.