Closed Bug 913548 Opened 6 years ago Closed 6 years ago

nsExceptionHandler.cpp:623:15 [-Wunused-but-set-variable] variable 'ignored' set but not used

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(3 files)

Build warning:
{
toolkit/crashreporter/nsExceptionHandler.cpp:623:15 [-Wunused-but-set-variable] variable 'ignored' set but not used
}

The code in question:
> 622       // not much we can do in case of error
> 623       ssize_t ignored = sys_write(fd, crashReporterAPIData->get(),
> 624                                   crashReporterAPIData->Length());
> 625       ignored = sys_write(fd, kCrashTimeParameter, kCrashTimeParameterLen);
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp

In other functions where we have this "ignored" variable, we have (void)ignored -- e.g. 
> 515     if (fd != -1) {
> 516       ssize_t ignored = sys_write(fd, crashTimeString, crashTimeStringLen);
> 517       (void)ignored;

Presumably we need that here, too.

(Or, better, we could switch them all to "unused <<".)
This switches existing "(void)" casts (a non-standard GCC-introduced idiom, IIUC) over to "unused<<" (which is more guaranteed-to-work-everywhere).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #800841 - Flags: review?(ted)
Here's a "diff -w" version of part 1, for maybe-easier-reviewing.
Attachment #800841 - Flags: review?(ted) → review+
...and here's the main patch, nuking the unused "ignored" variable and replacing it with "unused <<".

I also added some unused<< to the sys_write calls below it, whose value was previously not captured. (Those were the only sys_write calls in this file whose value wasn't captured, so I'm assuming they should be made consistent with the rest.  If we decide that we can just directly call sys_write in this file, then we can change all of them in a followup. But for now, I'm assuming we need the unused<< to avoid triggering build warnings on some platform.)
Attachment #800847 - Flags: review?(ted)
Comment on attachment 800847 [details] [diff] [review]
913548-part2.patch

Review of attachment 800847 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +645,1 @@
>        }        

Can you kill this whitespace while you're here?
Attachment #800847 - Flags: review?(ted) → review+
Sure. I added that whitespace-fix locally. Thanks for the quick review turnaround!

I'm doing a builds-only Try run, as a sanity-check before landing:
  https://tbpl.mozilla.org/?tree=Try&rev=5af1e7082eb3
Apparently I forgot about this bug.

The Try run from Comment 5 isn't showing any results on the TBPL interface (probably because it's a month back in time :)), but the buildapi interface shows that all the builds were green:
 https://secure.pub.build.mozilla.org/buildapi/self-serve/try/rev/5af1e7082eb3

So I'll go ahead and land this, this evening.
https://hg.mozilla.org/mozilla-central/rev/53ef1d805b5b
https://hg.mozilla.org/mozilla-central/rev/f0ed1e2c7ca2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.