From 9da40379230003595430c9df9c2da8510ef79977 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Tue, 14 Jul 2020 14:27:42 +0300 Subject: [PATCH 01/12] Added `Face` move mechanism --- src/Face.cu | 39 +++++++++++++++++++++++++++++++++++---- src/Face.cuh | 38 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/Face.cu b/src/Face.cu index e8c4b10..cada80f 100644 --- a/src/Face.cu +++ b/src/Face.cu @@ -11,6 +11,7 @@ __device__ SpacePoint calculate_normal(const SpacePoint *vertices) return normal / get_distance(normal, origin); } + __device__ Face::Face(const SpacePoint *vertices, int n_of_vertices) : vertices(malloc_and_copy(vertices, n_of_vertices)), n_of_vertices(n_of_vertices), normal(calculate_normal(vertices)), node(nullptr) @@ -18,12 +19,42 @@ __device__ Face::Face(const SpacePoint *vertices, int n_of_vertices) : } -__device__ Face::Face(const Face &other) : - vertices(malloc_and_copy(other.vertices, other.n_of_vertices)), - n_of_vertices(other.n_of_vertices), - normal(other.normal), node(other.node) +__device__ Face &Face::operator=(const Face &other) +{ + if(this != &other) + { + vertices = malloc_and_copy(other.vertices, other.n_of_vertices); + n_of_vertices = other.n_of_vertices; + normal = other.normal; + node = nullptr; + } + return *this; +} + +__device__ Face::Face(const Face &other) +{ + *this = other; +} + +__device__ Face &Face::operator=(Face &&other) noexcept +{ + if(this != &other) + { + swap(vertices, other.vertices); + swap(n_of_vertices, other.n_of_vertices); + swap(normal, other.normal); + swap(node, other.node); + } + + return *this; +} + +__device__ Face::Face(Face &&other) noexcept { + // Protection of further destruction: + vertices = nullptr; + *this = std::move(other); } __device__ Face::~Face() diff --git a/src/Face.cuh b/src/Face.cuh index 70fd1ac..4319dbc 100644 --- a/src/Face.cuh +++ b/src/Face.cuh @@ -19,7 +19,12 @@ class MapNode; class Polyhedron; -/// Object describing a polyhedron face +/** + * Object describing a polyhedron face + * + * @warning Before using the class, please, carefully read documentation for both copy and move constructors and + * assignment operators! + */ class Face { public: @@ -37,9 +42,38 @@ public: */ __device__ Face(const SpacePoint *vertices, int n_of_vertices); - /// `Face` object copy constructor + /** + * `Face` object copy assignment operator + * + * @warning When copying `Face` object, its `node` field is not copied, but is set to `nullptr`, which means + * the new `Face` won't have a node attached. Please, be careful + */ + __device__ Face &operator=(const Face &other); + + /** + * `Face` object copy constructor + * + * @warning When copying `Face` object, its `node` field is not copied, but is set to `nullptr`, which means + * the new `Face` won't have a node attached. Please, be careful + */ __device__ Face(const Face &other); + /** + * `Face` object move assignment operator + * + * @warning If the being moved `Face`'s field `node` is not `nullptr`, this might mean there is a node pointing + * to the `Face` being moved, so moving it will invalidate the pointers set at `*node` + */ + __device__ Face &operator=(Face &&other) noexcept; + + /** + * Face` object move constructor + * + * @warning If the being moved `Face`'s field `node` is not `nullptr`, this might mean there is a node pointing + * to the `Face` being moved, so moving it will invalidate the pointers set at `*node` + */ + __device__ Face(Face &&other) noexcept; + /// Destructs a `Face` object __device__ ~Face(); From 5d4f3cccd46a0db369e37403d0832d18e3bc2972 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 7 Aug 2020 15:31:52 +0300 Subject: [PATCH 02/12] Minor includes optimization + bugfix `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 --- src/Face.cu | 9 ++++----- src/Face.cuh | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Face.cu b/src/Face.cu index cada80f..e6a2a2a 100644 --- a/src/Face.cu +++ b/src/Face.cu @@ -1,8 +1,7 @@ -#include - #include "Face.cuh" #include "MapNode.cuh" #include "Polyhedron.cuh" +#include "common.cuh" __device__ SpacePoint calculate_normal(const SpacePoint *vertices) @@ -40,6 +39,9 @@ __device__ Face &Face::operator=(Face &&other) noexcept { if(this != &other) { + // Protection for further destruction + vertices = nullptr; + swap(vertices, other.vertices); swap(n_of_vertices, other.n_of_vertices); swap(normal, other.normal); @@ -51,9 +53,6 @@ __device__ Face &Face::operator=(Face &&other) noexcept __device__ Face::Face(Face &&other) noexcept { - // Protection of further destruction: - vertices = nullptr; - *this = std::move(other); } diff --git a/src/Face.cuh b/src/Face.cuh index 4319dbc..f231062 100644 --- a/src/Face.cuh +++ b/src/Face.cuh @@ -3,7 +3,7 @@ #include "SpacePoint.cuh" -#include "common.cuh" + /** * Returns the normal to faces defined by vertices From 08cbe2de5bba21b32a7286c8f2bc507b0d46d161 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 7 Aug 2020 15:49:46 +0300 Subject: [PATCH 03/12] Added Polyhedron copy and move mechanisms --- src/Face.cu | 1 + src/Polyhedron.cu | 55 ++++++++++++++++++++++++++++++++++++++++---- src/Polyhedron.cuh | 45 ++++++++++++++++++++++++++++++++---- src/SimulationMap.cu | 2 +- 4 files changed, 93 insertions(+), 10 deletions(-) diff --git a/src/Face.cu b/src/Face.cu index e6a2a2a..7145196 100644 --- a/src/Face.cu +++ b/src/Face.cu @@ -53,6 +53,7 @@ __device__ Face &Face::operator=(Face &&other) noexcept __device__ Face::Face(Face &&other) noexcept { + // std::move is required here. Without it, copy assignment operator is called *this = std::move(other); } diff --git a/src/Polyhedron.cu b/src/Polyhedron.cu index f9d8841..5bdf44b 100644 --- a/src/Polyhedron.cu +++ b/src/Polyhedron.cu @@ -1,4 +1,5 @@ #include "Polyhedron.cuh" +#include "common.cuh" __device__ Polyhedron::Polyhedron(Face *faces, int n_of_faces) : @@ -7,11 +8,47 @@ __device__ Polyhedron::Polyhedron(Face *faces, int n_of_faces) : } +__device__ Polyhedron &Polyhedron::operator=(const Polyhedron &other) +{ + if(this != &other) + { + faces = malloc_and_copy(other.faces, other.n_of_faces); + n_of_faces = other.n_of_faces; + } + return *this; +} + +__device__ Polyhedron::Polyhedron(const Polyhedron &other) +{ + *this = other; +} + +__device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept +{ + if(this != &other) + { + // Protection for further destruction + faces = nullptr; + + swap(faces, other.faces); + swap(n_of_faces, other.n_of_faces); + } + + return *this; +} + +__device__ Polyhedron::Polyhedron(Polyhedron &&other) noexcept +{ + // std::move is required here. Without it, copy assignment operator is called + *this = std::move(other); +} + __device__ Polyhedron::~Polyhedron() { free((void *)faces); } + __device__ Face *Polyhedron::find_face_by_point(SpacePoint point) const { for(int i = 0; i < n_of_faces; ++i) @@ -25,6 +62,16 @@ __device__ Face *Polyhedron::find_face_by_point(SpacePoint point) const return &faces[0]; } +__device__ Face *Polyhedron::get_faces() const +{ + return faces; +} + +__device__ int Polyhedron::get_n_of_faces() const +{ + return n_of_faces; +} + __device__ bool does_edge_belong_to_face(SpacePoint a, SpacePoint b, const Face *face) { @@ -41,12 +88,12 @@ __device__ bool does_edge_belong_to_face(SpacePoint a, SpacePoint b, const Face __device__ Face *find_face_next_to_edge(int vertex_id, Face *current_face, Polyhedron *polyhedron) { - for(int i = 0; i < polyhedron->n_of_faces; ++i) - if(polyhedron->faces[i] != *current_face && + for(int i = 0; i < polyhedron->get_n_of_faces(); ++i) + if(polyhedron->get_faces()[i] != *current_face && does_edge_belong_to_face(current_face->get_vertices()[vertex_id], current_face->get_vertices()[vertex_id + 1], - &polyhedron->faces[i])) - return &polyhedron->faces[i]; + &polyhedron->get_faces()[i])) + return &polyhedron->get_faces()[i]; return current_face; } diff --git a/src/Polyhedron.cuh b/src/Polyhedron.cuh index a3ed7b5..3ae3490 100644 --- a/src/Polyhedron.cuh +++ b/src/Polyhedron.cuh @@ -4,7 +4,6 @@ #include "SpacePoint.cuh" #include "Face.cuh" -#include "common.cuh" /// Object describing a geometric polyhedron for the simulation @@ -19,8 +18,39 @@ public: */ __device__ Polyhedron(Face *faces, int n_of_faces); - /// Forbids copying `Polyhedron` objects - __host__ __device__ Polyhedron(const Polyhedron &) = delete; + /** + * `Polyhedron` object copy assignment operator + * + * @warning Copying `Polyhedron` object requires copying `Face`s inside it, which can sometimes lead to unexpected + * results (different from when moving!). For details check the documentation of `Face` copy assignment + * operator + */ + __device__ Polyhedron &operator=(const Polyhedron &other); + + /** + * `Polyhedron` object copy constructor + * + * @warning Copying `Polyhedron` object requires copying `Face`s inside it, which can sometimes lead to unexpected + * results (different from when moving!). For details check the documentation of `Face` copy constructor + */ + __device__ Polyhedron(const Polyhedron &other); + + /** + * `Polyhedron` object move assignment operator + * + * @warning Moving `Polyhedron` object requires moving `Face`s inside it, which can sometimes lead to unexpected + * results (different from when copying!). For details check the documentation of `Face` move assignment + * operator + */ + __device__ Polyhedron &operator=(Polyhedron &&other) noexcept; + + /** + * `Polyhedron` object move constructor + * + * @warning Moving `Polyhedron` object requires moving `Face`s inside it, which can sometimes lead to unexpected + * results (different from when copying!). For details check the documentation of `Face` move constructor + */ + __device__ Polyhedron(Polyhedron &&other) noexcept; /// Destructs a `Polyhedron` object __device__ ~Polyhedron(); @@ -35,12 +65,17 @@ public: */ __device__ Face *find_face_by_point(SpacePoint point) const; + __device__ Face *get_faces() const; + + __device__ int get_n_of_faces() const; + +private: /// Pointer-represented array of polyhedron faces - Face *const faces; + Face *faces; /// Number of polyhedron faces - const int n_of_faces; + int n_of_faces; }; diff --git a/src/SimulationMap.cu b/src/SimulationMap.cu index e43532b..28cab2b 100644 --- a/src/SimulationMap.cu +++ b/src/SimulationMap.cu @@ -23,7 +23,7 @@ __device__ SimulationMap::SimulationMap(Polyhedron *polyhedron) : { bool create_new_nodes = true; // New nodes are allowed to be created - Face *start_face = &polyhedron->faces[0]; + Face *start_face = &polyhedron->get_faces()[0]; SpacePoint start_node_coordinates = (start_face->get_vertices()[0] + start_face->get_vertices()[1] + start_face->get_vertices()[2]) / 3; nodes = (MapNode *)malloc(sizeof(MapNode)); From 17c845c657b85e0ae8ef8b38b2d8da797fc1f9fa Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 7 Aug 2020 17:48:24 +0300 Subject: [PATCH 04/12] Added `Particle` move mechanism --- src/Particle.cuh | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/Particle.cuh b/src/Particle.cuh index 50431c6..a666d7a 100644 --- a/src/Particle.cuh +++ b/src/Particle.cuh @@ -21,9 +21,28 @@ public: */ __device__ Particle(MapNode *map_node, double angle); - /// Forbids copying `Particle` objects + /** + * `Particle` object copy assignment operator (deleted) + * + * Deleted because copying a `Particle` object does not make sense in the model. (when implementing a fission you + * should construct each new `Particle` object from scratch) + */ + __host__ __device__ Particle &operator=(const Particle &) = delete; + + /** + * `Particle` object copy constructor (deleted) + * + * Deleted because copying a `Particle` object does not make sense in the model. (when implementing a fission you + * should construct each new `Particle` object from scratch) + */ __host__ __device__ Particle(const Particle &) = delete; + /// `Particle` object move assignment operator + /*__host__ __device__*/ Particle &operator=(Particle &&other) noexcept = default; + + /// `Particle` object move constructor + /* __host__ __device__*/ Particle(Particle &&other) noexcept = default; + /** * Moves particle in the direction in which the particle is rotated @@ -107,8 +126,8 @@ private: /** - * Flag showing whether the particle was processed during current iteration (should be changed with `capture` and - * `release` methods) + * Flag showing whether the particle was processed during current simulation iteration (should only be updated with + * `capture` and `release` methods) */ bool is_captured = false; }; From e6bdd4c0d0072ebf4ada262230f5d4a2a5655a11 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 7 Aug 2020 20:37:59 +0300 Subject: [PATCH 05/12] Forbade both copying and moving `SimulationMap` --- src/SimulationMap.cuh | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/SimulationMap.cuh b/src/SimulationMap.cuh index d800a3b..c5e7d25 100644 --- a/src/SimulationMap.cuh +++ b/src/SimulationMap.cuh @@ -18,9 +18,42 @@ public: */ __device__ explicit SimulationMap(Polyhedron *polyhedron); - /// Forbids copying `SimulationMap` objects + /** + * `SimulationMap` object copy assignment operator (deleted) + * + * Deleted because despite `SimulationMap` makes sense and is possible to implement, accidental copying of a + * `SimulationMap` may seriously harm performance and need to copy it is a rather special case. However, a special + * function for copying `SimulationMap` objects will possibly be implemented someday + */ + __host__ __device__ SimulationMap &operator=(const SimulationMap &other) = delete; + + /** + * `SimulationMap` object copy constructor (deleted) + * + * Deleted because despite `SimulationMap` makes sense and is possible to implement, accidental copying of a + * `SimulationMap` may seriously harm performance and need to copy it is a rather special case. However, a special + * function for copying `SimulationMap` objects will possibly be implemented someday + */ __host__ __device__ SimulationMap(const SimulationMap &) = delete; + /** + * `SimulationMap` object move assignment operator (deleted) + * + * Deleted because there is no need in this operation yet, and the default operator can't be used, because it would + * break some implementation invariants. Deletion of this function is not a design decision, the team just + * doesn't want to spend time on it's implementation + */ + __host__ __device__ SimulationMap &operator=(SimulationMap &&other) noexcept = delete; + + /** + * `SimulationMap` object move constructor (deleted) + * + * Deleted because there is no need in this operation yet, and the default constructor can't be used, because it + * would break some implementation invariants. Deletion of this function is not a design decision, the team just + * doesn't want to spend time on it's implementation + */ + __host__ __device__ SimulationMap(SimulationMap &&other) noexcept = delete; + /// Destructs a `SimulationMap` object __device__ ~SimulationMap(); @@ -46,9 +79,6 @@ public: /// Pointer-represented array of nodes on the map MapNode *nodes; - /// Pointer to the polyhedron simulation is running on - Polyhedron *const polyhedron; - private: /** * Returns coordinates of neighbor node with or without projection on polyhedron @@ -118,6 +148,9 @@ private: bool create_new_nodes); + /// Pointer to the polyhedron simulation is running on + Polyhedron *polyhedron; + /// The number of nodes on the map int n_of_nodes; }; From f04a32c7e4361628f71b799c2181c1a93c47e42f Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 7 Aug 2020 21:27:58 +0300 Subject: [PATCH 06/12] `MapNode` minor improvements --- src/MapNode.cu | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/MapNode.cu b/src/MapNode.cu index 6ec55ef..27826a3 100644 --- a/src/MapNode.cu +++ b/src/MapNode.cu @@ -4,8 +4,8 @@ __device__ MapNode::MapNode(Polyhedron *const polyhedron, Face *polyhedron_face, SpacePoint coordinates) : - polyhedron(polyhedron), trail(0), contains_food(false), coordinates(coordinates), - polyhedron_face(polyhedron_face), left(nullptr), top(nullptr), right(nullptr), bottom(nullptr) + trail(0), temp_trail(0), left(nullptr), top(nullptr), right(nullptr), bottom(nullptr), polyhedron(polyhedron), + polyhedron_face(polyhedron_face), coordinates(coordinates), contains_food(false), particle(nullptr) {} __device__ MapNode::~MapNode() @@ -27,7 +27,7 @@ __device__ MapNode::~MapNode() */ __device__ inline bool set_neighbor(MapNode **target, MapNode *value) { - static_assert(sizeof(target) <= sizeof(unsigned long long *), "I think, I can't safely cast `MapNode **` to" + static_assert(sizeof(target) <= sizeof(unsigned long long *), "I think, I can't safely cast `MapNode **` to " "`unsigned long long *`"); if(value == nullptr) @@ -109,7 +109,7 @@ __device__ bool MapNode::does_contain_particle() const __device__ bool MapNode::attach_particle(Particle *p) { - static_assert(sizeof(&particle) <= sizeof(unsigned long long *), "I think, I can't safely cast `Particle **` to" + static_assert(sizeof(&particle) <= sizeof(unsigned long long *), "I think, I can't safely cast `Particle **` to " "`unsigned long long *`"); return nullptr == (Particle *)atomicCAS((unsigned long long *)&particle, (unsigned long long)nullptr, @@ -128,7 +128,7 @@ __device__ void MapNode::detach_particle() __device__ bool MapNode::detach_particle(Particle *p) { - static_assert(sizeof(&particle) <= sizeof(unsigned long long *), "I think, I can't safely cast `Particle **` to" + static_assert(sizeof(&particle) <= sizeof(unsigned long long *), "I think, I can't safely cast `Particle **` to " "`unsigned long long *`"); return p == (Particle *)atomicCAS((unsigned long long *)&particle, (unsigned long long)p, From cafffe63af4fa632ccd9b83e7de0b8b1a5ddb843 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Sat, 8 Aug 2020 15:35:00 +0300 Subject: [PATCH 07/12] Updated `MapNode` move/copy mechanisms --- src/MapNode.cu | 28 ++++++++++++++++++++++++++++ src/MapNode.cuh | 47 +++++++++++++++++++++++------------------------ 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/MapNode.cu b/src/MapNode.cu index 27826a3..f2c2ab7 100644 --- a/src/MapNode.cu +++ b/src/MapNode.cu @@ -8,6 +8,34 @@ __device__ MapNode::MapNode(Polyhedron *const polyhedron, Face *polyhedron_face, polyhedron_face(polyhedron_face), coordinates(coordinates), contains_food(false), particle(nullptr) {} +__host__ __device__ MapNode &MapNode::operator=(MapNode &&other) noexcept +{ + if(this != &other) + { + // Protection for further destruction + particle = nullptr; + + swap(polyhedron, other.polyhedron); + swap(trail, other.trail); + swap(temp_trail, other.temp_trail); + swap(left, other.left); + swap(top, other.top); + swap(right, other.right); + swap(bottom, other.bottom); + swap(polyhedron_face, other.polyhedron_face); + swap(coordinates, other.coordinates); + swap(contains_food, other.contains_food); + swap(particle, other.particle); + } + + return *this; +} + +__host__ __device__ MapNode::MapNode(MapNode &&other) noexcept +{ + *this = std::move(other); +} + __device__ MapNode::~MapNode() { delete particle; diff --git a/src/MapNode.cuh b/src/MapNode.cuh index 2f2a565..4a3b9c7 100644 --- a/src/MapNode.cuh +++ b/src/MapNode.cuh @@ -29,32 +29,31 @@ public: */ __device__ MapNode(Polyhedron *const polyhedron, Face *polyhedron_face, SpacePoint coordinates); - /// Forbids copying `MapNode` objects + /** + * `MapNode` object copy assignment operator (deleted) + * + * Deleted because the need to copy a `MapNode` is a rather special case and can be done with other methods, while + * an accidental copying of a `MapNode` can result into unexpected things (for example, when copying a `MapNode` + * the `Particle` from the original node shouldn't be copied, and unwanted copy may create an impression that + * there is no particle in a node, but in fact it will be checking a copy of an original node) + */ + __host__ __device__ MapNode &operator=(const MapNode &other) = delete; + + /** + * `MapNode` object copy constructor (deleted) + * + * Deleted because the need to copy a `MapNode` is a rather special case and can be done with other methods, while + * an accidental copying of a `MapNode` can result into unexpected things (for example, when copying a `MapNode` + * the `Particle` from the original node shouldn't be copied, and unwanted copy may create an impression that + * there is no particle in a node, but in fact it will be checking a copy of an original node) + */ __host__ __device__ MapNode(const MapNode &) = delete; - /// Move assignment operator - __host__ __device__ MapNode &operator=(MapNode &&other) noexcept - { - swap(polyhedron, other.polyhedron); - swap(trail, other.trail); - swap(temp_trail, other.temp_trail); - swap(left, other.left); - swap(right, other.right); - swap(top, other.top); - swap(bottom, other.bottom); - swap(polyhedron_face, other.polyhedron_face); - swap(coordinates, other.coordinates); - swap(contains_food, other.contains_food); - swap(particle, other.particle); - return *this; - } - - /// Move constructor for MapNode - __host__ __device__ MapNode(MapNode &&other) noexcept - { - particle = nullptr; - *this = std::move(other); - } + /// `MapNode` object move assignment operator + __host__ __device__ MapNode &operator=(MapNode &&other) noexcept; + + /// `MapNode` object move constructor + __host__ __device__ MapNode(MapNode &&other) noexcept; /// Destructs a `MapNode` object __device__ ~MapNode(); From d9b1a0872cd0f373383e721f0caceca044810d88 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Sat, 8 Aug 2020 15:52:09 +0300 Subject: [PATCH 08/12] Forbade moving `Particle` objects --- src/Particle.cuh | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Particle.cuh b/src/Particle.cuh index a666d7a..f424261 100644 --- a/src/Particle.cuh +++ b/src/Particle.cuh @@ -37,11 +37,21 @@ public: */ __host__ __device__ Particle(const Particle &) = delete; - /// `Particle` object move assignment operator - /*__host__ __device__*/ Particle &operator=(Particle &&other) noexcept = default; + /** + * `Particle` object move assignment operator (deleted) + * + * Deleted because `Particle` should be created by a `MapNode` which saves pointers to the created particle. Moving + * a `Particle` invalidates the `MapNode`'s pointer + */ + __host__ __device__ Particle &operator=(Particle &&other) noexcept = delete; - /// `Particle` object move constructor - /* __host__ __device__*/ Particle(Particle &&other) noexcept = default; + /** + * `Particle` object move constructor (deleted) + * + * Deleted because `Particle` should be created by a `MapNode` which saves pointers to the created particle. Moving + * a `Particle` invalidates the `MapNode`'s pointer + */ + __host__ __device__ Particle(Particle &&other) noexcept = delete; /** From f8a9b046b2c0999582d8c88fe2bf3fdd57713c6e Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 28 Aug 2020 12:39:47 +0300 Subject: [PATCH 09/12] Minor documentation improvement --- src/SimulationMap.cuh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/SimulationMap.cuh b/src/SimulationMap.cuh index c5e7d25..3c04aa2 100644 --- a/src/SimulationMap.cuh +++ b/src/SimulationMap.cuh @@ -21,7 +21,7 @@ public: /** * `SimulationMap` object copy assignment operator (deleted) * - * Deleted because despite `SimulationMap` makes sense and is possible to implement, accidental copying of a + * Deleted because despite copying `SimulationMap` makes sense and is possible to implement, accidental copying of a * `SimulationMap` may seriously harm performance and need to copy it is a rather special case. However, a special * function for copying `SimulationMap` objects will possibly be implemented someday */ @@ -30,7 +30,7 @@ public: /** * `SimulationMap` object copy constructor (deleted) * - * Deleted because despite `SimulationMap` makes sense and is possible to implement, accidental copying of a + * Deleted because despite copying `SimulationMap` makes sense and is possible to implement, accidental copying of a * `SimulationMap` may seriously harm performance and need to copy it is a rather special case. However, a special * function for copying `SimulationMap` objects will possibly be implemented someday */ From d24294cba88912e6b0a2d4ce00e036981f0be879 Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 28 Aug 2020 12:51:05 +0300 Subject: [PATCH 10/12] Added `SimulationMap` move mechanism --- src/SimulationMap.cu | 21 ++++++++++++++++++++- src/SimulationMap.cuh | 20 ++++---------------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/SimulationMap.cu b/src/SimulationMap.cu index 28cab2b..af1c398 100644 --- a/src/SimulationMap.cu +++ b/src/SimulationMap.cu @@ -101,6 +101,26 @@ __device__ SimulationMap::SimulationMap(Polyhedron *polyhedron) : } } +__host__ __device__ SimulationMap &SimulationMap::operator=(SimulationMap &&other) noexcept +{ + if(this != &other) + { + // Protection for further destruction + nodes = nullptr; + + swap(n_of_nodes, other.n_of_nodes); + swap(nodes, other.nodes); + swap(polyhedron, other.polyhedron); + } + + return *this; +} + +__host__ __device__ SimulationMap::SimulationMap(SimulationMap &&other) noexcept +{ + *this = std::move(other); +} + __device__ SimulationMap::~SimulationMap() { free(nodes); @@ -211,4 +231,3 @@ __global__ void get_n_of_nodes(const SimulationMap *const simulation_map, int *r *return_value = simulation_map->get_n_of_nodes(); } - diff --git a/src/SimulationMap.cuh b/src/SimulationMap.cuh index 3c04aa2..7a5a1b3 100644 --- a/src/SimulationMap.cuh +++ b/src/SimulationMap.cuh @@ -36,23 +36,11 @@ public: */ __host__ __device__ SimulationMap(const SimulationMap &) = delete; - /** - * `SimulationMap` object move assignment operator (deleted) - * - * Deleted because there is no need in this operation yet, and the default operator can't be used, because it would - * break some implementation invariants. Deletion of this function is not a design decision, the team just - * doesn't want to spend time on it's implementation - */ - __host__ __device__ SimulationMap &operator=(SimulationMap &&other) noexcept = delete; + /// `SimulationMap` object move assignment operator + __host__ __device__ SimulationMap &operator=(SimulationMap &&other) noexcept; - /** - * `SimulationMap` object move constructor (deleted) - * - * Deleted because there is no need in this operation yet, and the default constructor can't be used, because it - * would break some implementation invariants. Deletion of this function is not a design decision, the team just - * doesn't want to spend time on it's implementation - */ - __host__ __device__ SimulationMap(SimulationMap &&other) noexcept = delete; + /// `SimulationMap` object move constructor + __host__ __device__ SimulationMap(SimulationMap &&other) noexcept; /// Destructs a `SimulationMap` object __device__ ~SimulationMap(); From 40b94e4eec47542272a6852e237aba192e2af98e Mon Sep 17 00:00:00 2001 From: Nikolay Nechaev Date: Fri, 28 Aug 2020 12:55:13 +0300 Subject: [PATCH 11/12] Removed some strange comments inside of move constructors/assignment operators No idea why I have written them. They look redundant and even strange in somme places --- src/Face.cu | 2 -- src/MapNode.cu | 1 - src/Polyhedron.cu | 2 -- src/SimulationMap.cu | 1 - 4 files changed, 6 deletions(-) diff --git a/src/Face.cu b/src/Face.cu index 7145196..6723433 100644 --- a/src/Face.cu +++ b/src/Face.cu @@ -39,7 +39,6 @@ __device__ Face &Face::operator=(Face &&other) noexcept { if(this != &other) { - // Protection for further destruction vertices = nullptr; swap(vertices, other.vertices); @@ -53,7 +52,6 @@ __device__ Face &Face::operator=(Face &&other) noexcept __device__ Face::Face(Face &&other) noexcept { - // std::move is required here. Without it, copy assignment operator is called *this = std::move(other); } diff --git a/src/MapNode.cu b/src/MapNode.cu index f2c2ab7..0e4a6de 100644 --- a/src/MapNode.cu +++ b/src/MapNode.cu @@ -12,7 +12,6 @@ __host__ __device__ MapNode &MapNode::operator=(MapNode &&other) noexcept { if(this != &other) { - // Protection for further destruction particle = nullptr; swap(polyhedron, other.polyhedron); diff --git a/src/Polyhedron.cu b/src/Polyhedron.cu index 5bdf44b..ae33d26 100644 --- a/src/Polyhedron.cu +++ b/src/Polyhedron.cu @@ -27,7 +27,6 @@ __device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept { if(this != &other) { - // Protection for further destruction faces = nullptr; swap(faces, other.faces); @@ -39,7 +38,6 @@ __device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept __device__ Polyhedron::Polyhedron(Polyhedron &&other) noexcept { - // std::move is required here. Without it, copy assignment operator is called *this = std::move(other); } diff --git a/src/SimulationMap.cu b/src/SimulationMap.cu index af1c398..721b1c7 100644 --- a/src/SimulationMap.cu +++ b/src/SimulationMap.cu @@ -105,7 +105,6 @@ __host__ __device__ SimulationMap &SimulationMap::operator=(SimulationMap &&othe { if(this != &other) { - // Protection for further destruction nodes = nullptr; swap(n_of_nodes, other.n_of_nodes); From 0117a54269999db1709f59c01add70e75c6502d3 Mon Sep 17 00:00:00 2001 From: tanya-kta Date: Fri, 11 Sep 2020 23:47:14 +0300 Subject: [PATCH 12/12] Fixed issue found during the review --- src/Face.cu | 4 ++-- src/Face.cuh | 2 +- src/MapNode.cu | 4 ++-- src/Polyhedron.cu | 4 ++-- src/SimulationMap.cu | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Face.cu b/src/Face.cu index 6723433..e97a4de 100644 --- a/src/Face.cu +++ b/src/Face.cu @@ -39,8 +39,6 @@ __device__ Face &Face::operator=(Face &&other) noexcept { if(this != &other) { - vertices = nullptr; - swap(vertices, other.vertices); swap(n_of_vertices, other.n_of_vertices); swap(normal, other.normal); @@ -52,6 +50,8 @@ __device__ Face &Face::operator=(Face &&other) noexcept __device__ Face::Face(Face &&other) noexcept { + vertices = nullptr; + *this = std::move(other); } diff --git a/src/Face.cuh b/src/Face.cuh index f231062..299e275 100644 --- a/src/Face.cuh +++ b/src/Face.cuh @@ -67,7 +67,7 @@ public: __device__ Face &operator=(Face &&other) noexcept; /** - * Face` object move constructor + * `Face` object move constructor * * @warning If the being moved `Face`'s field `node` is not `nullptr`, this might mean there is a node pointing * to the `Face` being moved, so moving it will invalidate the pointers set at `*node` diff --git a/src/MapNode.cu b/src/MapNode.cu index 0e4a6de..65117d8 100644 --- a/src/MapNode.cu +++ b/src/MapNode.cu @@ -12,8 +12,6 @@ __host__ __device__ MapNode &MapNode::operator=(MapNode &&other) noexcept { if(this != &other) { - particle = nullptr; - swap(polyhedron, other.polyhedron); swap(trail, other.trail); swap(temp_trail, other.temp_trail); @@ -32,6 +30,8 @@ __host__ __device__ MapNode &MapNode::operator=(MapNode &&other) noexcept __host__ __device__ MapNode::MapNode(MapNode &&other) noexcept { + particle = nullptr; + *this = std::move(other); } diff --git a/src/Polyhedron.cu b/src/Polyhedron.cu index ae33d26..f0d0872 100644 --- a/src/Polyhedron.cu +++ b/src/Polyhedron.cu @@ -27,8 +27,6 @@ __device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept { if(this != &other) { - faces = nullptr; - swap(faces, other.faces); swap(n_of_faces, other.n_of_faces); } @@ -38,6 +36,8 @@ __device__ Polyhedron &Polyhedron::operator=(Polyhedron &&other) noexcept __device__ Polyhedron::Polyhedron(Polyhedron &&other) noexcept { + faces = nullptr; + *this = std::move(other); } diff --git a/src/SimulationMap.cu b/src/SimulationMap.cu index 721b1c7..93c2077 100644 --- a/src/SimulationMap.cu +++ b/src/SimulationMap.cu @@ -105,8 +105,6 @@ __host__ __device__ SimulationMap &SimulationMap::operator=(SimulationMap &&othe { if(this != &other) { - nodes = nullptr; - swap(n_of_nodes, other.n_of_nodes); swap(nodes, other.nodes); swap(polyhedron, other.polyhedron); @@ -117,6 +115,8 @@ __host__ __device__ SimulationMap &SimulationMap::operator=(SimulationMap &&othe __host__ __device__ SimulationMap::SimulationMap(SimulationMap &&other) noexcept { + nodes = nullptr; + *this = std::move(other); }