From 8c44793228750537c08ea7b19fc18df0138f9501 Mon Sep 17 00:00:00 2001 From: Sergey Sharybin Date: Wed, 23 Mar 2022 12:05:47 +0100 Subject: Implement C++ methods for DNA structures This change makes it possible to add implementation of common C++ methods for DNA structures which helps ensuring unsafe operations like shallow copy are done explicitly. For example, creating a shallow copy used to be: Object temp_object = *input_object; In the C++ context it was seen like the temp_object is properly decoupled from the input object, while in the reality is it not. Now this code becomes: Object temp_object = blender::dna::shallow_copy(*input_object); The copy and move constructor and assignment operators are now explicitly disabled. Other than a more explicit resource management this change also solves a lot of warnings generated by the implicitly defined copy constructors w.r.t dealing with deprecated fields. These warnings were generated by Apple Clang when a shallow object copy was created via implicitly defined copy constructor. In order to enable C++ methods for DNA structures a newly added macro `DNA_DEFINE_CXX_METHODS()` is to be used: tpyedef struct Object { DNA_DEFINE_CXX_METHODS(Object) ... } Object; For the shallow copy use `blender::dna::shallow_copy()`. The implementation of the memcpy is hidden via an internal DNA function to avoid pulling `string.h` into every DNA header. This means that the solution does not affect on the headers dependencies. --- Ideally `DNA_shallow_copy` would be defined in a more explicit header, but don;t think we have a suitable one already. Maybe we can introduce `DNA_access.h` ? Differential Revision: https://developer.blender.org/D14427 --- source/blender/blenkernel/intern/mesh_convert.cc | 6 +- source/blender/blenkernel/intern/object.cc | 2 +- .../depsgraph/intern/depsgraph_query_iter.cc | 2 +- source/blender/editors/object/object_transform.cc | 4 +- .../io/wavefront_obj/exporter/obj_export_mesh.cc | 2 +- source/blender/makesdna/DNA_defs.h | 81 ++++++++++++++++++++++ source/blender/makesdna/DNA_object_types.h | 2 + source/blender/makesdna/intern/dna_utils.c | 12 ++++ source/blender/makesdna/intern/makesdna.c | 12 ++++ 9 files changed, 116 insertions(+), 7 deletions(-) diff --git a/source/blender/blenkernel/intern/mesh_convert.cc b/source/blender/blenkernel/intern/mesh_convert.cc index 40c6fbcf67e..1be2d06ce61 100644 --- a/source/blender/blenkernel/intern/mesh_convert.cc +++ b/source/blender/blenkernel/intern/mesh_convert.cc @@ -910,7 +910,8 @@ static void curve_to_mesh_eval_ensure(Object &object) * * So we create temporary copy of the object which will use same data as the original bevel, but * will have no modifiers. */ - Object bevel_object = {{nullptr}}; + Object bevel_object; + memset(&bevel_object, 0, sizeof(bevel_object)); if (curve.bevobj != nullptr) { memcpy(&bevel_object, curve.bevobj, sizeof(bevel_object)); BLI_listbase_clear(&bevel_object.modifiers); @@ -919,7 +920,8 @@ static void curve_to_mesh_eval_ensure(Object &object) } /* Same thing for taper. */ - Object taper_object = {{nullptr}}; + Object taper_object; + memset(&taper_object, 0, sizeof(taper_object)); if (curve.taperobj != nullptr) { memcpy(&taper_object, curve.taperobj, sizeof(taper_object)); BLI_listbase_clear(&taper_object.modifiers); diff --git a/source/blender/blenkernel/intern/object.cc b/source/blender/blenkernel/intern/object.cc index 1e3b5d77fa7..6d1eac5af49 100644 --- a/source/blender/blenkernel/intern/object.cc +++ b/source/blender/blenkernel/intern/object.cc @@ -3946,7 +3946,7 @@ bool BKE_object_minmax_dupli(Depsgraph *depsgraph, /* pass */ } else { - Object temp_ob = *dob->ob; + Object temp_ob = blender::dna::shallow_copy(*dob->ob); /* Do not modify the original boundbox. */ temp_ob.runtime.bb = nullptr; BKE_object_replace_data_on_shallow_copy(&temp_ob, dob->ob_data); diff --git a/source/blender/depsgraph/intern/depsgraph_query_iter.cc b/source/blender/depsgraph/intern/depsgraph_query_iter.cc index 5788e8efa07..d5566e4be98 100644 --- a/source/blender/depsgraph/intern/depsgraph_query_iter.cc +++ b/source/blender/depsgraph/intern/depsgraph_query_iter.cc @@ -167,7 +167,7 @@ bool deg_iterator_duplis_step(DEGObjectIterData *data) /* Temporary object to evaluate. */ Object *dupli_parent = data->dupli_parent; Object *temp_dupli_object = &data->temp_dupli_object; - *temp_dupli_object = *dob->ob; + *temp_dupli_object = blender::dna::shallow_copy(*dob->ob); temp_dupli_object->base_flag = dupli_parent->base_flag | BASE_FROM_DUPLI; temp_dupli_object->base_local_view_bits = dupli_parent->base_local_view_bits; temp_dupli_object->runtime.local_collections_bits = diff --git a/source/blender/editors/object/object_transform.cc b/source/blender/editors/object/object_transform.cc index 24425b5a991..afd2c048379 100644 --- a/source/blender/editors/object/object_transform.cc +++ b/source/blender/editors/object/object_transform.cc @@ -1695,13 +1695,13 @@ static void object_apply_rotation(Object *ob, const float rmat[3][3]) static void object_apply_location(Object *ob, const float loc[3]) { /* quick but weak */ - Object ob_prev = *ob; + Object ob_prev = blender::dna::shallow_copy(*ob); float mat[4][4]; copy_m4_m4(mat, ob->obmat); copy_v3_v3(mat[3], loc); BKE_object_apply_mat4(ob, mat, true, true); copy_v3_v3(mat[3], ob->loc); - *ob = ob_prev; + *ob = blender::dna::shallow_copy(ob_prev); copy_v3_v3(ob->loc, mat[3]); } diff --git a/source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc b/source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc index aa63e65b1e8..8c9b04a5ac3 100644 --- a/source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc +++ b/source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc @@ -33,7 +33,7 @@ OBJMesh::OBJMesh(Depsgraph *depsgraph, const OBJExportParams &export_params, Obj { /* We need to copy the object because it may be in temporary space. */ Object *obj_eval = DEG_get_evaluated_object(depsgraph, mesh_object); - export_object_eval_ = *obj_eval; + export_object_eval_ = dna::shallow_copy(*obj_eval); export_mesh_eval_ = export_params.apply_modifiers ? BKE_object_get_evaluated_mesh(&export_object_eval_) : BKE_object_get_pre_modified_mesh(&export_object_eval_); diff --git a/source/blender/makesdna/DNA_defs.h b/source/blender/makesdna/DNA_defs.h index b4230209dd5..283eacf1a16 100644 --- a/source/blender/makesdna/DNA_defs.h +++ b/source/blender/makesdna/DNA_defs.h @@ -46,3 +46,84 @@ /* non-id name variables should use this length */ #define MAX_NAME 64 + +/* #DNA_DEFINE_CXX_METHODS is used to define C++ methods which are needed for proper/safe resource + * management, making unsafe (from an ownership perspective: i.e. pointers which sometimes needs to + * be set to nullptr on copy, sometimes needs to be dupalloc-ed) operations explicit, and taking + * care of compiler specific warnings when dealing with members marked with DNA_DEPRECATED. + * + * The `class_name` argument is to match the structure name the macro is used from. + * + * Typical usage example: + * + * typedef struct Object { + * DNA_DEFINE_CXX_METHODS(Object) + * } Object; + */ +#ifndef __cplusplus +# define DNA_DEFINE_CXX_METHODS(class_name) +#else + +/* Forward-declared here since there is no simple header file to be pulled for this functionality. + * Avoids pulling `string.h` from this header to get access to #memcpy. */ +extern "C" void _DNA_internal_memcpy(void *dst, const void *src, size_t size); + +namespace blender::dna::internal { + +template class ShallowDataConstRef { + public: + constexpr explicit ShallowDataConstRef(const T &ref) : ref_(ref) + { + } + + inline const T *get_pointer() const + { + return &ref_; + } + + private: + const T &ref_; +}; + +} // namespace blender::dna::internal + +# define DNA_DEFINE_CXX_METHODS(class_name) \ + class_name() = default; \ + ~class_name() = default; \ + /* Delete copy and assignment, which are not safe for resource ownership. */ \ + class_name(const class_name &other) = delete; \ + class_name(class_name &&other) noexcept = delete; \ + class_name &operator=(const class_name &other) = delete; \ + class_name &operator=(class_name &&other) = delete; \ + /* Support for shallow copy. */ \ + class_name(const blender::dna::internal::ShallowDataConstRef ref) \ + { \ + _DNA_internal_memcpy(this, ref.get_pointer(), sizeof(class_name)); \ + } \ + class_name &operator=(const blender::dna::internal::ShallowDataConstRef ref) \ + { \ + if (this != ref.get_pointer()) { \ + _DNA_internal_memcpy(this, ref.get_pointer(), sizeof(class_name)); \ + } \ + return *this; \ + } + +namespace blender::dna { + +/* Creates shallow copy of the given object. + * The entire object is copied as-is using memory copy. + * + * Typical usage: + * Object temp_object = blender::dna::shallow_copy(*input_object); + * + * From the implementation detail go via copy constructor/assign operator defined in the structure. + */ +template +[[nodiscard]] inline internal::ShallowDataConstRef shallow_copy(const T &other) +{ + return internal::ShallowDataConstRef(other); +} + +} // namespace blender::dna + +#endif diff --git a/source/blender/makesdna/DNA_object_types.h b/source/blender/makesdna/DNA_object_types.h index 9e0bf7dcc5a..c3708e25ee7 100644 --- a/source/blender/makesdna/DNA_object_types.h +++ b/source/blender/makesdna/DNA_object_types.h @@ -233,6 +233,8 @@ enum eObjectLineArt_Flags { }; typedef struct Object { + DNA_DEFINE_CXX_METHODS(Object) + ID id; /** Animation data (must be immediately after id for utilities to use it). */ struct AnimData *adt; diff --git a/source/blender/makesdna/intern/dna_utils.c b/source/blender/makesdna/intern/dna_utils.c index 8c86ef69ebd..560b91b925e 100644 --- a/source/blender/makesdna/intern/dna_utils.c +++ b/source/blender/makesdna/intern/dna_utils.c @@ -308,3 +308,15 @@ const char *DNA_struct_rename_legacy_hack_alias_from_static(const char *name) } /** \} */ + +/* -------------------------------------------------------------------- */ +/** \name Internal helpers for C++ + * \{ */ + +void _DNA_internal_memcpy(void *dst, const void *src, size_t size); +void _DNA_internal_memcpy(void *dst, const void *src, const size_t size) +{ + memcpy(dst, src, size); +} + +/** \} */ diff --git a/source/blender/makesdna/intern/makesdna.c b/source/blender/makesdna/intern/makesdna.c index 0d2f265a9b5..12ec7262906 100644 --- a/source/blender/makesdna/intern/makesdna.c +++ b/source/blender/makesdna/intern/makesdna.c @@ -620,6 +620,7 @@ static int preprocess_include(char *maindata, const int maindata_len) int newlen = 0; comment = 0; a = maindata_len; + bool skip_until_closing_brace = false; while (a--) { if (cp[0] == '/' && cp[1] == '*') { @@ -646,6 +647,17 @@ static int preprocess_include(char *maindata, const int maindata_len) a -= 13; cp += 13; } + else if (match_identifier(cp, "DNA_DEFINE_CXX_METHODS")) { + /* single values are skipped already, so decrement 1 less */ + a -= 21; + cp += 21; + skip_until_closing_brace = true; + } + else if (skip_until_closing_brace) { + if (cp[0] == ')') { + skip_until_closing_brace = false; + } + } else { md[0] = cp[0]; md++; -- cgit v1.2.3