ntamas / plfit

Fitting power-law distributions to empirical data, according to the method of Clauset, Shalizi and Newman
GNU General Public License v2.0
47 stars 17 forks source link

Cleanup and fix some memory leaks #43

Closed szhorvat closed 10 months ago

szhorvat commented 10 months ago

This carries over all changes that were made in igraph, not just https://github.com/igraph/igraph/pull/2433 There's a fix for void parameter lists.

Some of the headers I removed on the igraph side were actually necessary here (for rand()).

I tested that this compiled with -std=gnu89 and -std=c99, but it does not compile with -std=c89. That's because plfit uses many functions which were present as a GNU extension for a long time, but only became standard in C99.

szhorvat commented 10 months ago

@ntamas It might be a good idea to check the Clang Analyzer results on this package. Do it like this:

cmake .. -GXcode
open plfit.xcodeproj

The use Product -> Analyze.

There are "Left operand of > is a garbage value" warnings in plfit.c, which I think come from omitting the error check from the plfit_estimate_alpha_continuous() and plfit_continuous() calls. But we can't do the error check when OpenMP is enabled, right?

Then there are some null pointer dereference warnings in lbfgs.c, but I'm not sure they are correct ...

ntamas commented 10 months ago

I'll do the static analysis and then check whether I can fix any more of those warnings.

Error checks with OpenMP are indeed problematic -- you cannot bail out in the middle of an OpenMP section but you need to coordinate until you get back to the main thread and then you can do the cleanup there. I'll look into this.

ntamas commented 10 months ago

Okay, FYI, I went through the static analysis results.

szhorvat commented 10 months ago
  • As you said, the reason for the warnings in plfit.c is the lack of PLFIT_CHECK() calls. I can probably work around this by having a shared plfit_error_t shared_error variable declared outside the loop and a per-thread plfit_error_t error that updates shared_error if needed. If I manage to fix this, I'll add a commit to master.

I wouldn't spend much time on this right now, as the errors are probably OOM only, and if that happens, the program is likely to crash anyway.

At one point we'll need to figure out a proper way to do error checks from within OpenMP code and start adding OpenMP to igraph itself. That will require careful thought and it's likely to be a lot of work.