Nodes created in ConcurrentInsertOnlyHashMap have implicit ownership

[lyxell]

Currently the nodes created here https://github.com/souffle-lang/souffle/blob/96d557cf6fe43d54eeeb9241bfb1a0905f846745/src/include/souffle/datastructure/ConcurrentInsertOnlyHashMap.h#L170-L178 have implicit ownership, i.e. the developer/reader must manually make sure that memory is always correctly freed. It would be nice if this ownership could be made explicit using std::unique_ptr or its type alias souffle::Own.

Ownership for these nodes are transferred here under some circumstances:

https://github.com/souffle-lang/souffle/blob/96d557cf6fe43d54eeeb9241bfb1a0905f846745/src/include/souffle/datastructure/ConcurrentInsertOnlyHashMap.h#L204-L242

This might be tricky to implement with explicit ownership but it should hopefully be possible.

[quentin]

Hi, This datastructure is designed specifically to be paired with ConcurrentFlyweight. That explain why the interface may not look like something we would use standalone. Maybe it would be preferable to relocate it outside of datastructure.

[lyxell]

Hi Quentin,

Even though the datastructure is designed to be paired with ConcurrentFlyweight I still think there could be worth in improving the interface to ensure that the implementation does not suffer from memory leaks. As a reader it is currently very hard to trust or manually verify that this implementation is leak-free due to its complexity and use of operator new and operator delete.

Clang-tidy can also be configured to catch this (the related clang-tidy rule can be found here based on this guideline).

[SanaaHamel]

A trivial fix is to wrap it in a unique_ptr. I've a commit for this that I'm hoping to land once my current ugly brood of PRs have been dealt with.