Closed
Bug 1123527
Opened 8 years ago
Closed 8 years ago
Fix problems found by cppcheck
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(8 files, 1 obsolete file)
2.63 KB,
patch
|
ben.tian
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
glandium
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
jhford
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
ehsan.akhgari
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
mcmanus
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1000 bytes,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
874 bytes,
patch
|
jesup
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•8 years ago
|
||
cppcheck found this. Not important, but doesn't hurt to fix it.
Attachment #8551570 -
Flags: review?(jhford)
![]() |
Assignee | |
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1bdd395b2d https://hg.mozilla.org/integration/mozilla-inbound/rev/42df041f636c
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8551563 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8551568 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Keywords: leave-open
![]() |
Assignee | |
Comment 8•8 years ago
|
||
Attachment #8551593 -
Flags: review?(mcmanus)
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ec1bdd395b2d https://hg.mozilla.org/mozilla-central/rev/42df041f636c
Comment 10•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8551593 -
Flags: review?(mcmanus) → review+
Comment 11•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 12•8 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 13•8 years ago
|
||
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
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8551570 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8551571 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8551593 -
Flags: checkin+
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa7dbc827709 https://hg.mozilla.org/mozilla-central/rev/34fe493c5806 https://hg.mozilla.org/mozilla-central/rev/7da2cd7a1125
![]() |
Assignee | |
Comment 15•8 years ago
|
||
cppcheck found this.
Attachment #8552837 -
Flags: review?(dholbert)
Comment 17•8 years ago
|
||
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+
Updated•8 years ago
|
Version: unspecified → Trunk
Comment 18•8 years ago
|
||
(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)
![]() |
Assignee | |
Comment 20•8 years ago
|
||
Attachment #8552860 -
Flags: review?(jwatt)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8552848 -
Attachment is obsolete: true
Attachment #8552848 -
Flags: review?(jwatt)
![]() |
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(n.nethercote)
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8552837 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8552859 -
Flags: checkin+
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8552860 -
Flags: checkin+
Comment 23•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9083ef23526a https://hg.mozilla.org/mozilla-central/rev/79010299a931 https://hg.mozilla.org/mozilla-central/rev/b64e7b3d671a
![]() |
Assignee | |
Comment 24•8 years ago
|
||
I've done as much here as I'm going to.
You need to log in
before you can comment on or make changes to this bug.
Description
•