1123527 - Fix problems found by cppcheck

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix syntax error in BT_WARNING and BT_LOGD

cppcheck identified the extra ')' at the end of these macros. This code must
not be compiled in any of our standard configurations.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Added #error cases for impossible platforms

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.

Comment 3

[Ben Tian (inactive)]

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Fix a trivial leak

cppcheck found this. Not important, but doesn't hurt to fix it.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Initialized some locals in TestNoArithmeticExprInArgument.cpp

I think these undefined variables don't matter because it's a static analysis
input, but defining them does stop cppcheck from complaining.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1bdd395b2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/42df041f636c

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Avoid a leak on realloc failure in nsMultiMixedConv.cpp

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec1bdd395b2d
https://hg.mozilla.org/mozilla-central/rev/42df041f636c

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 8551570 [details] [diff] [review]
Fix a trivial leak

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

Cool!

Comment 11

[:ehsan akhgari]

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. ;-)

Comment 12

[Nicholas Nethercote [inactive]]

> 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.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7dbc827709
https://hg.mozilla.org/integration/mozilla-inbound/rev/34fe493c5806
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da2cd7a1125

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa7dbc827709
https://hg.mozilla.org/mozilla-central/rev/34fe493c5806
https://hg.mozilla.org/mozilla-central/rev/7da2cd7a1125

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: Fix syntax error when NS_COORD_IS_FLOAT is defined

cppcheck found this.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Fix syntax errors when DEBUG_LAYOUT is defined

cppcheck found these.

Comment 17

[Daniel Holbert [:dholbert]]

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

Comment 18

[Daniel Holbert [:dholbert]]

(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?

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: Fix syntax error when ERR_REPORTING_SYSLOG is defined

cppcheck found this.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: Fix syntax errors when DEBUG_LAYOUT is defined

Comment 21

[Daniel Holbert [:dholbert]]

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)

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9083ef23526a
https://hg.mozilla.org/mozilla-central/rev/79010299a931
https://hg.mozilla.org/mozilla-central/rev/b64e7b3d671a

Comment 24

[Nicholas Nethercote [inactive]]

I've done as much here as I'm going to.