If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TestSTLWrappers.cpp:28: warning: unused variable ‘unused’

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dholbert, Assigned: cjones)

Tracking

Trunk
mozilla1.9.3a5
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
> ../../../mozilla/xpcom/tests/TestSTLWrappers.cpp: In function ‘int main()’:
> ../../../mozilla/xpcom/tests/TestSTLWrappers.cpp:28: warning: unused variable ‘unused’

Guilty code:
> 26     TRY {
> 27       // this should abort; NOT throw an exception
> 28       int unused = v.at(1);
> 29     } CATCH(const std::out_of_range& e) {
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestSTLWrappers.cpp

We should either
 (A) Nix the unused variable, changing line 28 to "v.at(1);"
 (B) Add a line after 28 saying "(void)unused;" to suppress the warning that the variable is unused (as discussed in GCC manpage under -Wunused-value).

cjones, preferences?
Created attachment 439072 [details] [diff] [review]
Remove unused variable warning and add extra failure check
Assignee: nobody → jones.chris.g
Attachment #439072 - Flags: review?(dholbert)
(Reporter)

Comment 2

8 years ago
Comment on attachment 439072 [details] [diff] [review]
Remove unused variable warning and add extra failure check

> int main() {
>     std::vector<int> v;
> 
>     TRY {
>       // this should abort; NOT throw an exception
>-      int unused = v.at(1);
>+      return v.at(1);

Why *return* the value here? (when we're not expecting to be there in the first place)?  Is that just to "use" it?  That seems broken...  In particular:
 - in the (unexpected) situation where v.at(1) succeeds, we'd immediately return and pass the test (right?) when we really want it to fail.
 - the early-return makes the final chunk of code ("didn't abort?") completely unreachable, AFAICT.

I'd prefer one of the options that I outlined in comment 0 over this -- I think they're both more explicit about what we're doing.

(The other changes in this patch look fine to me, though.)
Attachment #439072 - Flags: review?(dholbert) → review-
(In reply to comment #2)
> (From update of attachment 439072 [details] [diff] [review])
> I'd prefer one of the options that I outlined in comment 0 over this -- I think
> they're both more explicit about what we're doing.
> 

I'm allergic to |(void)x|, and although |v.at(1);| shouldn't be optimized away, I think it's bad style to write expression-y statements for side effects.

I have a third solution that should please all parties, sec ;).
Created attachment 439093 [details] [diff] [review]
Remove unused variable warning and add extra failure check, v2
Attachment #439072 - Attachment is obsolete: true
Attachment #439093 - Flags: review?(dholbert)
(Reporter)

Comment 5

8 years ago
Comment on attachment 439093 [details] [diff] [review]
Remove unused variable warning and add extra failure check, v2

>diff --git a/xpcom/tests/TestSTLWrappers.cpp b/xpcom/tests/TestSTLWrappers.cpp
>       // this should abort; NOT throw an exception
>-      int unused = v.at(1);
>+      rv += v.at(1) ? 1 : 2;

Since v is a vector of int's, why not just directly do:
>       rv += v.at(1);

It'd also be worth adding a comment explaining the point of doing that addition, like this:
>  // Do some arithmetic with result of v.at() to avoid
>  // compiler warnings for unused variable/result.

r=dholbert with the above. :) Thanks for fixing this warning!
(Reporter)

Updated

8 years ago
Attachment #439093 - Flags: review?(dholbert) → review+
(Reporter)

Comment 6

8 years ago
(In reply to comment #5)
> Since v is a vector of int's, why not just directly do:
> >       rv += v.at(1);

Feel free to ignore this chunk, too, if you're worried about returning bogus values from main().  (I don't think it's worth worrying about, but I also don't particularly care *what* we do with v.at(1) as long as it doesn't cause warnings. :))
http://hg.mozilla.org/mozilla-central/rev/b2fc398be086
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a5
Iiuc what this test does, a 'TEST-PASS' line should be added before(.!.) |v.at(1)|, so we get it (and only it) if the test succeeds.
You need to log in before you can comment on or make changes to this bug.