cppcheck on Dillo source
Hi all, While waiting for the Tour De France to start on TV, I ran cppcheck on Dillo source for something to do - a few errors were picked up! I have compacted the findings (with two comments from me)and attached it as a plain text file. Keep up the great work guys and thanks! Nick -- Gosh that takes me back... or is it forward? That's the trouble with time travel, you never can tell." -- Doctor Who "Androids of Tara"
On Sat, 9 Jul 2016 12:35:10 +0100 Nick Warne <nick at linicks.net> wrote:
Hi all,
While waiting for the Tour De France to start on TV, I ran cppcheck on Dillo source for something to do - a few errors were picked up!
I have compacted the findings (with two comments from me)and attached it as a plain text file.
Keep up the great work guys and thanks!
Nick
OK, updating cppcheck to 1.74, I ran it again. A new error appears, but I don't know if it is a false positive, as I still don't really understand C++ (but do get the logic of function calls vs void etc.: Checking ../../slackbuilds/dillo/hg/dillo/src/ui.cc... [../../slackbuilds/dillo/hg/dillo/src/ui.cc:574]: (error) Return value of allocation function 'make_filemenu_button' is not stored. [../../slackbuilds/dillo/hg/dillo/src/ui.cc:588]: (error) Return value of allocation function 'make_filemenu_button' is not stored. Now, the call is: make_filemenu_button(); to: Fl_Widget *UI::make_filemenu_button() which returns (btn) so is that correct? Where does (btn) go in the calling function - or what does it do? Nick (trying to learn). -- Gosh that takes me back... or is it forward? That's the trouble with time travel, you never can tell." -- Doctor Who "Androids of Tara"
Hi, On Sat, Jul 09, 2016 at 03:28:25PM +0100, Nick Warne wrote:
On Sat, 9 Jul 2016 12:35:10 +0100 Nick Warne <nick at linicks.net> wrote:
Hi all,
While waiting for the Tour De France to start on TV, I ran cppcheck on Dillo source for something to do - a few errors were picked up!
I have compacted the findings (with two comments from me)and attached it as a plain text file.
Keep up the great work guys and thanks!
Nick
OK, updating cppcheck to 1.74, I ran it again. A new error appears, but I don't know if it is a false positive, as I still don't really understand C++ (but do get the logic of function calls vs void etc.:
Checking ../../slackbuilds/dillo/hg/dillo/src/ui.cc... [../../slackbuilds/dillo/hg/dillo/src/ui.cc:574]: (error) Return value of allocation function 'make_filemenu_button' is not stored. [../../slackbuilds/dillo/hg/dillo/src/ui.cc:588]: (error) Return value of allocation function 'make_filemenu_button' is not stored.
Now, the call is:
make_filemenu_button();
to:
Fl_Widget *UI::make_filemenu_button()
which returns (btn)
so is that correct? Where does (btn) go in the calling function - or what does it do?
Patch committed. It wasn't necessary anymore. Most probably it remained from a time when it was used. Now they are stored in local variables so it's safe to make the function void. -- Cheers Jorge.-
Hi Nick, On Sat, Jul 09, 2016 at 12:35:10PM +0100, Nick Warne wrote:
Hi all,
While waiting for the Tour De France to start on TV, I ran cppcheck on Dillo source for something to do - a few errors were picked up!
I have compacted the findings (with two comments from me)and attached it as a plain text file.
Thanks for the report. I personally didn't know of cppcheck. We have used several code cleanup tools in the past and we usually honour them as sometimes they catch important flaws. A bunch of patches was just committed. You may peruse them with "hg log -vp" for the details/explanations.
Checking ../../slackbuilds/dillo/hg/dillo/dpi/file.c... [../../slackbuilds/dillo/hg/dillo/dpi/file.c:356]: (error) syntax error
if S_ISDIR (finfo->mode) { Nick-> maybe?: if (S_ISDIR (finfo->mode)) {
Done.
Checking ../../slackbuilds/dillo/hg/dillo/dpid/dpid.c... [../../slackbuilds/dillo/hg/dillo/dpid/dpid.c:465]: (error) Resource leak: dpidrc_stream
return -1; Nick-> maybe?: return (-1);
Done.
Checking ../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc... [../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc:570]: (warning) %u in format string (no. 1) requires 'unsigned int *' but the argument type is 'int *'. Checking ../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc... [../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc:570]: (warning) %u in format string (no. 2) requires 'unsigned int *' but the argument type is 'int *'.
It may be a trivial s/%u/%d/, but after reading it a bit I'd prefer Sebastian to double check it. Not committed.
Checking ../../slackbuilds/dillo/hg/dillo/dw/iterator.cc... [../../slackbuilds/dillo/hg/dillo/dw/iterator.cc:124]: (error) Memory leak: eit3
Done. AFAICS, this is a false positive. Patched to please cppcheck anyway.
Checking ../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc... [../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc:654]: (error) Possible null pointer dereference: vloat
Well, the code should never reach there. Not changed. @Sebastian, BTW, coincidentally I'm using something like: SortedFloatsVector *list; list = isSubRefLeftFloat(ref) ? leftFloats : rightFloats; Float *vloat = list->get (getFloatIndexFromSubRef (ref)); in some patches I have, which also silence cppcheck. But maybe the original construct was meant for adding more options (like absolute positioned floats), but those may end in a third floats array, so I'm not sure. Is it safe to use the above construct, or should I leave it as is?
Checking ../../slackbuilds/dillo/hg/dillo/lout/container.cc... [../../slackbuilds/dillo/hg/dillo/lout/container.cc:143]: (error) Common realloc mistake: 'array' nulled but not freed upon failure [../../slackbuilds/dillo/hg/dillo/lout/container.cc:177]: (error) Common realloc mistake: 'array' nulled but not freed upon failure
Done.
Checking ../../slackbuilds/dillo/hg/dillo/lout/misc.cc... [../../slackbuilds/dillo/hg/dillo/lout/misc.cc:172]: (error) Common realloc mistake: 'bits' nulled but not freed upon failure
Done.
Truck loads of: Checking ../../slackbuilds/dillo/hg/dillo/src/md5.c: ARCH_IS_BIG_ENDIAN... [../../slackbuilds/dillo/hg/dillo/src/md5.c:216]: (error) Uninitialized variable: X in md5.c
Untouched: I'm not md5 savvy. Maybe corvid. -- Cheers Jorge.-
On Tue, 12 Jul 2016 15:02:31 -0400 Jorge Arellano Cid <jcid at dillo.org> wrote:
Hi Nick,
On Sat, Jul 09, 2016 at 12:35:10PM +0100, Nick Warne wrote:
Hi all,
While waiting for the Tour De France to start on TV, I ran cppcheck on Dillo source for something to do - a few errors were picked up!
I have compacted the findings (with two comments from me)and attached it as a plain text file.
Thanks for the report. I personally didn't know of cppcheck.
I have used it in the past - a few years ago, but as I say I was bored so found something to do...
We have used several code cleanup tools in the past and we usually honour them as sometimes they catch important flaws.
A bunch of patches was just committed. You may peruse them with "hg log -vp" for the details/explanations.
Good stuff. I watch the dillo hg site anyway as I like to see (and learn) all this C++ stuff.
It may be a trivial s/%u/%d/, but after reading it a bit I'd prefer Sebastian to double check it.
Not committed.
I looked and looked at that, but alas, don't understand why it's wrong/right :)
Checking ../../slackbuilds/dillo/hg/dillo/dw/iterator.cc... [../../slackbuilds/dillo/hg/dillo/dw/iterator.cc:124]: (error) Memory leak: eit3
Done.
AFAICS, this is a false positive. Patched to please cppcheck anyway.
Yes, even I got that, but wasn't sure.
Checking ../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc... [../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc:654]: (error) Possible null pointer dereference: vloat
Well, the code should never reach there. Not changed.
@Sebastian, BTW, coincidentally I'm using something like:
SortedFloatsVector *list; list = isSubRefLeftFloat(ref) ? leftFloats : rightFloats; Float *vloat = list->get (getFloatIndexFromSubRef (ref));
[../../slackbuilds/dillo/hg/dillo/src/md5.c:216]: (error) Uninitialized variable: X in md5.c
Untouched: I'm not md5 savvy. Maybe corvid.
Nor am I. I looked a lot at that code, and K[] whatever doesn't seem to be declared anywhere (with my little knowledge). Strange GCC doesn't squinny on that one? Thanks, Nick -- Gosh that takes me back... or is it forward? That's the trouble with time travel, you never can tell." -- Doctor Who "Androids of Tara"
Checking ../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc... [../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc:570]: (warning) %u in format string (no. 1) requires 'unsigned int *' but the argument type is 'int *'. Checking ../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc... [../../slackbuilds/dillo/hg/dillo/dw/hyphenator.cc:570]: (warning) %u in format string (no. 2) requires 'unsigned int *' but the argument type is 'int *'.
It may be a trivial s/%u/%d/, but after reading it a bit I'd prefer Sebastian to double check it.
Not committed.
I've looked at this, and this is rather Johannes' code. I'm not sure whether negative may cause troubles. Sebastian
On Di, Jul 12, 2016, Jorge Arellano Cid wrote:
Checking ../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc... [../../slackbuilds/dillo/hg/dillo/dw/ooffloatsmgr.cc:654]: (error) Possible null pointer dereference: vloat
Well, the code should never reach there. Not changed.
@Sebastian, BTW, coincidentally I'm using something like:
SortedFloatsVector *list; list = isSubRefLeftFloat(ref) ? leftFloats : rightFloats; Float *vloat = list->get (getFloatIndexFromSubRef (ref));
in some patches I have, which also silence cppcheck. But maybe the original construct was meant for adding more options (like absolute positioned floats), but those may end in a third floats array, so I'm not sure. Is it safe to use the above construct, or should I leave it as is?
OOFFloatsMgr only deals with floats, so it should be safe. However, I'd like to keep assertNotReached() there, to make debugging easier. Sebastian
participants (3)
-
jcid＠dillo.org -
nick＠linicks.net -
sgeerken＠dillo.org