azoitl
commented
5 days ago Thx for reporting this issue. While I agree that using a pointer to a std::vector entry is in this use case wrong, I also wanted to point out that in general storing pointers in vectors is not better. It is not using more memory allocations it using less. Also memory is more aligned with better cache locality, less allocation overhead as less allocations are performed, and what is even more important it is saver. Modern C++ compilers utilizing move semantics will even optimize away the object creation and deletion of the object that is created on the stack (no memory allocation is happening here). Therefore, especially for embedded memory constrained systems it is always better to not use pointers.
I would love if we could keep the none pointer version. But so far I have no idea how to do that.
In the localcomlayer.h the class CLocalCommGroupsManager stores the complete LocalComGroup objects in a vector. This not only makes unnecessary allocation and deletion of CLocalCommGroup objects when adding them to the vector but also crashes forte on Windows when running in debug. The resaon is, that CLocalComLayer::CLocalCommGroupsManager::findOrCreateLocalCommGroup() returns a pointer to this stored object which is then stored in the CLocalCommGroup mLocalCommGroup; member of the CLocalComLayer object. Now the following happens when insering the second entry to the mLocalCommGroups vector: The system reallocates the memory used for storing the mLocalCommGroups vector prior to adding the second element, so the extrenally stored pointer mLocalCommGroup to the first element gets invalid. This results in a crash when accessing the tangled pointer in mLocalCommGroup .
A solution is to not store the CLocalCommGroup object itself in the TLocalCommGroupList but only a pointer to it:
using TLocalCommGroupList = std::vector<CLocalCommGroup *>;