[Networkit] Possible bugs

Florian Weber uagws at student.kit.edu
Wed Jun 25 01:39:48 CEST 2014


Hi,

>   - auxiliary/NumberParsing.h, line 193: missing return

That one is clearly my fault and definitely a bug. Thanks for reviewing
the code.

Fixed in commit 2618.

>   - cpp/algebraic/Matrix.h: Subclasses of Matrix leak memory since
>     Matrix lacks a virtual destructor.

That one isn't my department, so just a few general comments (The
situation might be perfectly valid the way it is):

* Deleting through a non-virtual base-pointer isn't a leak but undefined
behavior → I would strongly recommend to consider code doing that as
release-blockers.
* Virtual Dtors are often overused: They rarely make much sense if you
don't have another virtual method; Publicly inheriting from a class
without vtable, sometimes IS a perfectly valid choice, but you should
really know what you are doing and should document your rationale.

Therefore: Everyone who uses a class derived from Matrix should review
whether they are deleting an instance through a Matrix-pointer. This
would be the case for `std::unique_ptr<Matrix>`; OTOH the following is
valid:

std::shared_ptr<Matrix> ptr = std::make_shared<DerivedMatrix>(g);

Note that the following is not valid though (and slower and terrible code):

std::shared_ptr<Matrix> ptr{new DerivedMatrix(g)};

If someone is unsure about his/her code, I'm willing to give that a
review in the next few days.


>   - properties/ParallelConnectedComponents.cpp lines 69 and 71: data
>     races writing change and nextActiveNodes. 

Definitely a critical bug.

>     This is unspecified behaviour 

I don't know about OpenMP, but I am quite sure that ISO-C++ actually
says that this is much worse: Data-races are completely undefined behavior.
(Hans Böhm at Going Native 12: “It's what we call catch-fire-semantics,
because your machine is allowed to catch fire. Even though, if there are
any hardware-architects in the audience: We discourage that.”)

Again: I would recommend to consider this as release-blocker.

>     I'd recommend using atomic_bool or #pragma omp atomic write.

I second the recommendation of std::atomic_bool.


>     Also see section 2.4 in 
> http://static.usenix.org/event/hotpar11/tech/final_files/Boehm.pdf.


Regards,
Florian

-------------- n�chster Teil --------------
Ein Dateianhang mit Bin�rdaten wurde abgetrennt...
Dateiname   : signature.asc
Dateityp    : application/pgp-signature
Dateigr��e  : 884 bytes
Beschreibung: OpenPGP digital signature
URL         : <https://lists.ira.uni-karlsruhe.de/mailman/private/networkit/attachments/20140625/b269d846/attachment.sig>


More information about the NetworKit mailing list