Closed Bug 1123527 Opened 5 years ago Closed 4 years ago

Fix problems found by cppcheck

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(8 files, 1 obsolete file)

cppcheck finds some interesting stuff that is worth fixing. Generally the fixes are tiny so I'm going to create a single bug report for lots of fixes rather than dozens of tiny bugs reports.
cppcheck identified the extra ')' at the end of these macros. This code must
not be compiled in any of our standard configurations.
Attachment #8551563 - Flags: review?(btian)
cppcheck says:

> b2g/app/nsBrowserApp.cpp:251: error: Uninitialized variable: gotCounters
> browser/app/nsBrowserApp.cpp:637: error: Uninitialized variable: gotCounters

It's a false positive because one of XP_WIN and XP_UNIX is always defined, but
it doesn't hurt to make that fact clearer.
Attachment #8551568 - Flags: review?(mh+mozilla)
Comment on attachment 8551563 [details] [diff] [review]
Fix syntax error in BT_WARNING and BT_LOGD

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

LGTM. Thanks for finding the nit.
Attachment #8551563 - Flags: review?(btian) → review+
cppcheck found this. Not important, but doesn't hurt to fix it.
Attachment #8551570 - Flags: review?(jhford)
I think these undefined variables don't matter because it's a static analysis
input, but defining them does stop cppcheck from complaining.
Attachment #8551571 - Flags: review?(ehsan)
Comment on attachment 8551568 [details] [diff] [review]
Added #error cases for impossible platforms

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

::: b2g/app/nsBrowserApp.cpp
@@ +223,5 @@
>  #elif defined(XP_WIN)
>    IO_COUNTERS ioCounters;
>    gotCounters = GetProcessIoCounters(GetCurrentProcess(), &ioCounters);
> +#else
> +  #error "unknown platform"

Capital U to unknown. Add a comment that this is to make cppcheck happy, because a casual reader might end up wanting to remove the useless #else if he doesn't know where it comes from.
Attachment #8551568 - Flags: review?(mh+mozilla) → review+
Attachment #8551563 - Flags: checkin+
Attachment #8551568 - Flags: checkin+
Keywords: leave-open
Comment on attachment 8551570 [details] [diff] [review]
Fix a trivial leak

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

Cool!
Attachment #8551570 - Flags: review?(jhford) → review+
Attachment #8551593 - Flags: review?(mcmanus) → review+
Comment on attachment 8551571 [details] [diff] [review]
Initialized some locals in TestNoArithmeticExprInArgument.cpp

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

Is there any way to instruct cppcheck to not look at the test files in this directory?  These files are never built, only syntax-parsed: <https://dxr.mozilla.org/mozilla-central/source/build/clang-plugin/tests/Makefile.in#5>

But I guess this is harmless (and useless. ;-)
Attachment #8551571 - Flags: review?(ehsan) → review+
> Is there any way to instruct cppcheck to not look at the test files in this
> directory?

Yes, the -i option can be used to ignore files and/or directories.
Attachment #8551570 - Flags: checkin+
Attachment #8551571 - Flags: checkin+
Attachment #8551593 - Flags: checkin+
cppcheck found this.
Attachment #8552837 - Flags: review?(dholbert)
cppcheck found these.
Attachment #8552848 - Flags: review?(jwatt)
Comment on attachment 8552837 [details] [diff] [review]
Fix syntax error when NS_COORD_IS_FLOAT is defined

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

Thanks! r=me
Attachment #8552837 - Flags: review?(dholbert) → review+
Version: unspecified → Trunk
(In reply to Nicholas Nethercote [:njn] from comment #16)
> Created attachment 8552848 [details] [diff] [review]
> Fix syntax errors when DEBUG_LAYOUT is defined

This is an empty patch. Forgot to qref?
Flags: needinfo?(n.nethercote)
cppcheck found this.
Attachment #8552859 - Flags: review?(rjesup)
Attachment #8552848 - Attachment is obsolete: true
Attachment #8552848 - Flags: review?(jwatt)
Flags: needinfo?(n.nethercote)
Comment on attachment 8552860 [details] [diff] [review]
Fix syntax errors when DEBUG_LAYOUT is defined

Stealing review, since I'm already here, and this is a trivial fix.

>--- a/layout/xul/nsBoxFrame.cpp
>@@ -1461,23 +1461,24 @@ nsBoxFrame::PaintXULDebugBackground(nsRe
>   r.y = r.y + r.height - debugBorder.bottom;
>   r.height = debugBorder.bottom;
>   drawTarget->FillRect(NSRectToRect(r, appUnitsPerDevPixel), color);
>   
>   // if we have dirty children or we are dirty 
>   // place a green border around us.
>   if (NS_SUBTREE_DIRTY(this)) {
>     nsRect dirty(inner);
>-    ColorPattern green(ToDeviceColor(Color0.f, 1.f, 0.f, 1.f)));
>+    ColorPattern green(ToDeviceColor(Color0.f, 1.f, 0.f, 1.f));
                                       ^^^^^^^^
You're removing a close-paren here, but really this is missing an open-paren (after "Color" in "Color0.f").

r=me with that open-paren added (and the removed close-paren restored)
Attachment #8552860 - Flags: review?(jwatt) → review+
Comment on attachment 8552859 [details] [diff] [review]
Fix syntax error when ERR_REPORTING_SYSLOG is defined

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

Sure, though we'd never define that.
Attachment #8552859 - Flags: review?(rjesup) → review+
Attachment #8552837 - Flags: checkin+
Attachment #8552859 - Flags: checkin+
Attachment #8552860 - Flags: checkin+
I've done as much here as I'm going to.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.