Move/copy mechanisms #8
Conversation
`vertices` field should be set to `nullptr`, because it will be used in the destructor and values other than `nullptr` can lead to undefined behaviour
|
Hi! Looks like our conversation with Codacy support has finished. They verified that this was an issue on Codacy side (fix of which is already committed but not yet deployed to production) and suggested ignoring the issues for now. The PR is absolutely ready for a review! |
kolayne
added
the
blocking
…operators No idea why I have written them. They look redundant and even strange in somme places
kolayne
force-pushed
the
move_copy_mechanisms
branch
from
acaf60c
to
40b94e4
Compare
src/Face.cuh
Outdated
| __device__ Face &operator=(Face &&other) noexcept; | ||
|
|
||
| /** | ||
| * Face` object move constructor |
There was a problem hiding this comment.
Should be back quote before Face`
| { | ||
| if(this != &other) | ||
| { | ||
| vertices = malloc_and_copy(other.vertices, other.n_of_vertices); |
There was a problem hiding this comment.
Field Face::vertices has type const SpacePoint. Other variables which were moved to private section of their classes have no const in their types or const was removed from the type. Does Face::vertices really need const in type?
There was a problem hiding this comment.
Please, note, that the actual type is const SpacePoint * (* is an important part). It is different from the const SpacePoint *const type (which would mean the field vertices can't be modified), but the actual time means that the content of the array can't be modified without replacing the whole array. If the field was const pointer, it would take away the opportunity to, for example, move a face into an existing one, but the pointer to const only prevents us from accidentally modifying the vertices array (vertices of an existing Face shouldn't change i. e. have a specific set of vertices, shouldn't they?). Because the field is private, it's already impossible to modify the array from outside of the Face class, but is still possible to modify it from inside, which shouldn't happen and is worth protecting.
The general rule is if you don't modify a variable, make it constant. This way you will get additional protection from your compiler
| @@ -46,9 +67,6 @@ public: | |||
| /// Pointer-represented array of nodes on the map | |||
| MapNode *nodes; | |||
There was a problem hiding this comment.
It has just drawn my attention, it might be not the problem of this PR. I suppose field SimulationMap::nodes should be moved to private, because it should not be changed after its creation in SimulationMap constructor
There was a problem hiding this comment.
I think this should be a separate issue
src/Polyhedron.cu
Outdated
| __device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept | ||
| { | ||
| if(this != &other) | ||
| { | ||
| faces = nullptr; |
There was a problem hiding this comment.
Polyhedron B has to be moved to Polyhedron A. If A was created earlier (move constructor is not called during the moving), then setting A.faces to nullptr will cause leak of memory that A.faces contained before moving B to A. Same to Face::vertices, MapNode::particle (I suppose) and SimulationMap::nodes
There was a problem hiding this comment.
Yes, you are right (by the way, I think, your explanation could be easier to understand if you'd provide a code snippet with an example). And yes, this does work for all Polyhedron::faces, Face::vertices, MapNode::particle and SimulationMap::nodes. Thank you for the alarm!
# Conflicts: # src/MapNode.cu
Added or restricted move/copy mechanisms for classes. Fixes #3
Is blocked by #6. The current branch (move_copy_mechanisms) is based onrefactoring_face_class, so the corresponding PR should be approved before approving this, and new commits atrefactoring_face_classshould be merged into the current branch if there are any