[Networkit] Possible bugs

Florian Weber uagws at student.kit.edu
Wed Jun 25 21:20:17 CEST 2014


Hi,


> 
> - community/test/CommunityGTest.cpp, lines 55, 57, 58, 414: Why do
> you use new and then deref immediately?
> 
> - distmeasures/test/DistMeasuresGTest.cpp, line 29
> 
> - generators/GeneratorsGTest.cpp, lines 34, 35, 59, 61, 88, 89 (Only
> G constructed in lines 36, 62 and 90 is deleted.)

This is just test-code, so not actually too bad. Nonetheless it would
certainly be a good idea to rework this.


> 
> - community/EPPFactory.cpp, lines 14, 16, 23, 25, 27, 30

I just took a look on that code. It definitely needs some refactoring there:

* EPPFactory should just be a function (maybe put it into the EPP-TU,
which would also likely decrease compile-time)
* Every single one of those pointers should most likely be replaced by a
std::unique_ptr
* Reflecting that, most of the reference-parameters should be replaced
with std::unique_ptr as well:

virtual void addBaseClusterer(CommunityDetectionAlgorithm&  base);

should look like this:

virtual void addBaseClusterer(
             std::unique_ptr<CommunityDetectionAlgorithm> base);

Personally I would recommend to add the following to the coding-standards:

* Never use allocating `new` outside the implementation of make_unique
* Never write your code in a way that forces you to free resources with
`delete`
* If you have to manage a resource use (or write) a class that does
nothing besides managing the resource for that

That being said: Fixing the code should be relatively easy and
straightforward.

> - generators/DynamicGraphSource.cpp, lines 44, 45

Basically: Same as above

> - community/test/CommunityDetectionBenchmark.cpp, lines 54, 71, 93,
> 115: You use the "lu" format specifier for printing a count 
> (uint64_t) with printf.The actual specifier is given by the macro 
> "PRIu64" in <cinttypes>. (Why do you use the type-unsafe printf
> anyway?)

To extend on that: If you want to print stuff, please use the
functionality in auxiliary/Log.h: Unlike printf

* it is thread-safe
* it is completely type-safe
* it can print more types than any other functionality (yes, this
includes printing a std::vector<std::map<std::pair<int, std::string>,
std::tuple<your_own_streamable_type, char*>>>, even though that would be
an insane data-structure)
* it can be activated/deactivated at both run- and compile-time
* it knows from where you call it and can show that
* it supports multiple log-levels
* allows to choose between using formatstrings (with ‘s’ as
format-specifier for every type) and just concatenating the arguments:

INFOF("my formatstring %s, %s ,%%", 42, std::vector<int>());
// -> "my formatstring 42, [], %\n"

INFO(23, "fnord"); // -> "23fnord\n"

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/cb5e3f54/attachment.sig>


More information about the NetworKit mailing list