hugin and cppcheck

[Lukáš Jirkovský]

Hi everyone,
I've just tried the cppcheck the open source tool for static analysis
of C++. It seems to be considered to be in some alpha state but it
seems that it works quite nicely. So for everyone interested I've
attached (file error_log.old) the result of running cppcheck -a on
hugin trunk. I'm still slowly going through the results.

It seems that it really detected some possible problems. I've tried to
fix them but I'd like to hear your opinion if it is really a problem
and if the solution is right.

svm.cpp:2738]: (error) Resource leak: fp
----------------------------------------------------
It's possible that due to short-circuiting the file will not be
properly closed. I'm not really sure if this can cause problems.
Here's my proposed fix:

Index: /home/lukas/development/hugin/src/celeste/svm.cpp
===================================================================
--- /home/lukas/development/hugin/src/celeste/svm.cpp (revision 4496)
+++ /home/lukas/development/hugin/src/celeste/svm.cpp (working copy)
@@ -2735,8 +2735,15 @@
}
fprintf(fp, "\n");
}
- if (ferror(fp) != 0 || fclose(fp) != 0) return -1;
- else return 0;
+ if (ferror(fp) != 0) {
+ fclose(fp);
+ return -1;
+ } else {
+ if (fclose(fp) != 0)
+ return -1;
+ else
+ return 0;
+ }
}

svm_model *svm_load_model(const char *model_file_name)

CelesteTrain.cpp:428]: (possible error) Memory leak: u_values
CelesteTrain.cpp:428]: (possible error) Memory leak: v_values
--------------------------------------------------------------------------------
This seems to be really an error because are allocated and never
deallocated. My proposed fix is:

Index: /home/lukas/development/hugin/src/celeste/training/CelesteTrain.cpp
===================================================================
--- /home/lukas/development/hugin/src/celeste/training/CelesteTrain.cpp (revision
4496)
+++ /home/lukas/development/hugin/src/celeste/training/CelesteTrain.cpp (working
copy)
@@ -414,6 +414,8 @@

delete[] response;
delete[] frameBuf;
+ delete[] u_values;
+ delete[] v_values;

// Export images
//exportImage(srcImageRange(in),
ImageExportInfo("output_images/image_reduced.jpg"));

regards,
Lukáš

[Tim Nugent]

Hi Lukáš,

Those Celeste fixes are fine, please commit them. I'll drop an email to
the libsvm developers with your patch.

Cheers,
Tim

[Lukáš Jirkovský]

Hi Tim,
thank you for response. I've committed both.

Lukas

2009/9/28 Tim Nugent <timn...@gmail.com>:

[Lukáš Jirkovský]

Committed another fix (rev 4516) but now I realized that the wxWidgets
may be deallocating the space automatically with the call to Close()
which I'm not 100% sure if it really calls only Close event or if it
destroys all data structures. Nevertheless hugin still works.

Index: /home/lukas/development/hugin/src/hugin1/hugin/MainFrame.cpp
===================================================================
--- /home/lukas/development/hugin/src/hugin1/hugin/MainFrame.cpp (revision 4516)
+++ /home/lukas/development/hugin/src/hugin1/hugin/MainFrame.cpp (revision 4514)
@@ -383,7 +383,6 @@

if(splash) {
splash->Close();
- delete splash;
}
wxYield();

Anyway, there are some reported memory leaks which I think they are
not really a problem like the leak in align_image_stack.cpp. Eg. this
one is that the leftImage is not deallocated but then
align_image_stack terminates almost immediately. Should I fix these
too?

Lukas

[Yuval Levy]

Lukáš Jirkovský wrote:
> Anyway, there are some reported memory leaks which I think they are
> not really a problem like the leak in align_image_stack.cpp. Eg. this
> one is that the leftImage is not deallocated but then
> align_image_stack terminates almost immediately. Should I fix these
> too?

Thanks for all the good work, Lukáš

Please fix. IMO keeping the code clean is good practice, always.

Also as a general practice: rely on your judgment when deciding whether
to commit or not. If you fear unintended consequences on platforms other
than yours, request people to test a patch. If you have no negative
feedback on the patch within a week or so, commit. And if the change is
small, works locally, and is unlikely to break other things, just commit it.

Yuv

[Lukáš Jirkovský]

2009/9/29 Yuval Levy <goo...@levy.ch>:

[hugin/src/hugin1/hugin/PreviewTool.h:36]: (error) Class PreviewTool
which is inherited by class PreviewIdentifyTool does not have a
virtual destructor

This one is not problem now since there doesn't seem to be any need
for destructor in any of it's children.
Is it possible that it may become a problem in future?

Lukas

[James Legg]

On Sun, 2009-10-11 at 16:43 +0200, Lukáš Jirkovský wrote:
> [hugin/src/hugin1/hugin/PreviewTool.h:36]: (error) Class PreviewTool
> which is inherited by class PreviewIdentifyTool does not have a
> virtual destructor
>
> This one is not problem now since there doesn't seem to be any need
> for destructor in any of it's children.
> Is it possible that it may become a problem in future?
>
> Lukas

There isn't a need for a destructor at the moment because the tools are
only freed when Hugin quits. Some PreviewTools allocate OpenGL textures
which are not freed automatically, so would need a destructor if that
changes.

I imagine more PreviewTools will be written, and we are planning to
change the way they are accessed (In the "Traditional Preview" thread),
so it is possible that a virtual destructor is needed in the future.

-James

[Lukáš Jirkovský]

Hi James,

2009/10/11 James Legg <lanky...@gmail.com>:

I can imagine new tools too. I'll leave the decision to you because
you're more experienced than I.

have a nice day,
Lukas

[James Legg]

I've added a virtual destructor to PreviewTool.

-James

[Yuval Levy]

thanks, guys! I've added all the fixes to the stable codeline. we'll
have at least one other release candidate before declaring 2009.4.0 final.

Yuv