Stop dereferencing end() iterators #1748
Conversation
|
|
||
| Edge &operator*() { return current; } | ||
| Edge &operator*() { | ||
| current = (*graph)[*pos]; |
There was a problem hiding this comment.
It seems like we don't need current anymore.
There was a problem hiding this comment.
That's what I thought initially, but you need it in order to be able to use references in the range for loops - for(auto &at: mol.atoms()).
There was a problem hiding this comment.
Right! I'm doing some sanity checks here just trying to figure out when the end was being dereferences as I didn't catch it initially with valgrind.
There was a problem hiding this comment.
Valgrind only showed it for me when I built in debug mode. I noticed it first in a debug build on windows, where the dereference led directly to a crash.
The problem itself was happening here:
https://github.com/rdkit/rdkit/blob/modern_cxx/Code/GraphMol/ROMol.h#L115
when this call is made:
https://github.com/rdkit/rdkit/blob/modern_cxx/Code/GraphMol/ROMol.h#L131
* cleanup the build requirements for test-valgrind * cleanup a recently added test * the new iterators would dereference end() iterators. Fix that.
The new code for doing range-based loops over atoms and bonds illegally dereferences the
end()vertex/edge iterators whenCXX{Atom,Bond}Iterator::end()is called. This fixes that.There's also a memory leak fix for one of the recently added tests.