PCL_COMMON: removing default constructors/destructors #3440

lightyear15 · 2019-10-24T21:09:24Z

This commit:

adds initiliazition for the non-static members
removes default constructors, destructors
removes from constructors all the calls to default member constructors

kunaltyagi · 2019-10-24T23:07:52Z

There are some merge conflicts. Please resolve them (in another commit)

kunaltyagi

Nice work! What do you think about extending the changes to all members in the classes you have touched?

common/include/pcl/TextureMesh.h

common/include/pcl/common/impl/accumulators.hpp

kunaltyagi · 2019-10-25T01:53:57Z

common/include/pcl/point_representation.h

      /** \brief Empty destructor */
-      virtual ~PointRepresentation () {}
+      virtual ~PointRepresentation () = default;
+      //TODO: check if copy and move constructors / assignment operators are needed


Shall we follow the rule of 5 here and just go ahead and default them? Will there be anything bad about it?

At a first glance I would say no. But I don't have a complete view of the entire code base

There are a few pitfalls in just declaring them defaults, specially in exception unsafe code. It might be better not to have them declared than to declare buggy defaults. Maybe a work for another PR

Please expand, why defaults are buggy? And also, how implicit declared destructor (i.e. destructor created by the compiler when user does not declare one) is any different by the default one?
What pitfalls are you referring to when code is not exception safe?

how implicit declared destructor ... is any different by the default one?

That's not what I was referring to. Your edit is perfect. I was commenting on my own statement of "what's the issue with blind rule of 5".

What pitfalls are you referring to when code is not exception safe?

Essentially, during copy, if there is an error, the rollback should result in the same state as before attempted copy. But with a) potentially self-copy b) copy of structs with resource owning semantics c) other similar corner cases, the rule of 5 might not be a good idea to apply blindly because the defaults do a element-wise operations. There might be scenarios where based on usage, we might have to do a copy into local variable before moving the resources and local variables to ensure that if copy fails, rollback is trivial.

I see your point - b), a) not really - nonetheless I would say that this might happen when your class has non-trivially copyable elements, but in the case of PODs or classes that hold std::strings or std::vector<int>s, probably rule of 5 ( or even 7 ) is just fine.
I know even std::string copy constructor might fail with std::bad_alloc, but again, running out of memory it's like an atomic bomb dropped in front of your house: who cares if the windows were closed or not (i.e. destructors are called or not ) .

We are meandering out of context of the PR. But this is as good place as any.

running out of memory it's like an atomic bomb dropped in front of your house

bad_alloc isn't called just due to OOM. It's also called when

a single allocation is larger than the one allowed by the allocator (eg: differing implementations of malloc). <-- most common reason

allocation takes too much time and it'll impact the "real-time"ness of the application. In soft real-time systems, page swaps required for a large allocation can take time which results in a throw (all custom malloc support maximum time to wait) <-- common in robotics

application has run out of it's quota of memory (not the OS). This approach is frequently used in sandboxes and extra memory is given to application to clean itself or to recover (via custom api calls) <-- common in cloud based virtualization

there's a custom memory management (FreeRTOS, distributed memory, etc.) which can throw bad_alloc for other reasons too <-- common in virtualization, hypervisors

Running out of memory is a very rare scenario for bad_alloc. Case 1 (common case) is essentially atom bomb for common users because it's not recoverable (by design or bad-calculations leading to the point) but not for all users.

common/include/pcl/point_cloud.h

common/include/pcl/common/vector_average.h

common/include/pcl/common/polynomial_calculations.h

common/include/pcl/PCLPointCloud2.h

lightyear15 · 2019-10-25T08:13:18Z

Nice work! What do you think about extending the changes to all members in the classes you have touched?

That was the idea, but unfortunately there are some stuff that need to be changed in order to accomplish it. Some matrices and vectors are initialized with identity matrices or other special matrices by calling static functions. AFAIK those functions need to be marked as constexpr first, then they can be used in member initialization.

common/include/pcl/correspondence.h

kunaltyagi · 2019-10-26T17:59:46Z

Force pushing every-time causes lack of knowledge of what's updated.

lightyear15 · 2019-10-26T18:24:14Z

Force pushing every-time causes lack of knowledge of what's updated.

sorry about that, next time I will make temporary commits, so that author or maintainer can squash them into one before merging

kunaltyagi · 2019-10-30T05:47:46Z

Apparently old compilers don't like the modifications wrt Eigen static functions and templates

common/include/pcl/common/vector_average.h:111:71: error: declaration of 'Eigen::Matrix<real, dimension, 1> pcl::VectorAverage<real, dimension>::dimension'
         Eigen::Matrix<real, dimension, 1> mean_ = Eigen::Matrix<real, dimension, 1>::Identity ();

I wonder what's going wrong in parsing. It's weird that the compiler thinks we are creating a variable dimension and not mean_

lightyear15 · 2019-10-30T09:28:07Z

Apparently old compilers don't like the modifications wrt Eigen static functions and templates
common/include/pcl/common/vector_average.h:111:71: error: declaration of 'Eigen::Matrix<real, dimension, 1> pcl::VectorAverage<real, dimension>::dimension'
         Eigen::Matrix<real, dimension, 1> mean_ = Eigen::Matrix<real, dimension, 1>::Identity ();
I wonder what's going wrong in parsing. It's weird that the compiler thinks we are creating a variable dimension and not mean_

I wonder if some simple aliasing, something like

using VectorType = Eigen::Matrix<real, dimension, 1>;
using MatrixType = Eigen::Matrix<real, dimension, dimension>;

would fix the problem. Because it really looks like a bug in the compiler, doesn't it?

kunaltyagi · 2019-10-30T11:47:58Z

Sounds like a good idea. Could you try that in the docker image (or the CI)? Image is pointcloudlibrary/env:16.04

kunaltyagi

LGTM. Please rebase and squash as appropriate

This commit: - adds initiliazition for the non-static members - removes default constructors, destructors - removes from constructors all the calls to default member constructors - adds aliases in vector_average to avoid odd errors in old compilers

SergioRAgostinho

I meant to review this PR but apparently was late to the party. I just highlighted a number of places where destructors should not be required.

SergioRAgostinho · 2019-11-04T08:51:50Z

common/include/pcl/common/vector_average.h

        //-----CONSTRUCTOR&DESTRUCTOR-----
        /** Constructor - dimension gives the size of the vectors to work with. */
        VectorAverage ();
        /** Destructor */
-        ~VectorAverage () {}
+        ~VectorAverage () = default;


Is there a reason for this destructor to not have been removed? I see no copy/move ctors/assignments.

SergioRAgostinho · 2019-11-04T08:58:18Z

common/include/pcl/common/time.h

@@ -65,6 +65,7 @@ namespace pcl

      /** \brief Destructor. */
      virtual ~StopWatch () = default;


This destructor can also be erased. It's a base class with no virtual methods.

SergioRAgostinho · 2019-11-04T09:00:08Z

common/include/pcl/common/transformation_from_correspondences.h

        { reset (); }

        /** Destructor */
-        ~TransformationFromCorrespondences () { };
+        ~TransformationFromCorrespondences () = default;


Same comment here. Base class, with no custom copy/move ctors/assignments nor virtual methods.

SergioRAgostinho · 2019-11-04T09:02:21Z

common/include/pcl/correspondence.h


    /** \brief Constructor. */
    inline Correspondence (int _index_query, int _index_match, float _distance) : 
      index_query (_index_query), index_match (_index_match), distance (_distance)
    {}

    /** \brief Empty destructor. */
-    virtual ~Correspondence () {}
+    virtual ~Correspondence () = default;


This one also does not require declaring the dtor at all.

SergioRAgostinho · 2019-11-04T09:03:21Z

common/include/pcl/correspondence.h

    /** \brief Empty destructor. */
-    virtual ~PointCorrespondence3D () {}
+    virtual ~PointCorrespondence3D () = default;


You can also remove this one.

SergioRAgostinho · 2019-11-04T09:03:31Z

common/include/pcl/correspondence.h

@@ -143,7 +140,8 @@ namespace pcl
    Eigen::Affine3f transformation;  //!< The transformation to go from the coordinate system
                                        //!< of point2 to the coordinate system of point1
    /** \brief Empty destructor. */
-    virtual ~PointCorrespondence6D () {}
+    virtual ~PointCorrespondence6D () = default;


And this one.

SergioRAgostinho · 2019-11-04T09:06:01Z

common/include/pcl/point_cloud.h


      /** \brief True if no points are invalid (e.g., have NaN or Inf values in any of their floating point fields). */
-      bool is_dense;
+      bool is_dense = true;


Note: PCLPointCloud2 is initialized with is_dense set to false but this one is set to true :/

Not nice, but probably also not critical. I think we should change the default in PCLPointCloud2 because hardly anyone constructs that type of point cloud "by hand".

Can you confirm what you wrote? You state one thing but your argument seems to point in the other direction. I interpreted what you wrote as

I think we should not change the default in PCLPointCloud2 because hardly anyone constructs that type of point cloud "by hand".

PCLPointCloud2 comes from ROS messages and these usually means actual sensors which naturally have nans, while the PointClouds are very often handcrafted. Which would actually mean, this was a well thought decision and their difference is intentional.

No, I indeed meant what I wrote. My thought is that since PCLPointCloud2 comes from ROS message, the default-constructed value of this field does not make any difference, it will be set to whatever value the message had. Similarly, when you call toPCLPointCloud2() to create a blob point cloud,is_dense will be set to that of the source cloud.

SergioRAgostinho · 2019-11-04T09:10:29Z

common/include/pcl/point_cloud.h

      {}

      /** \brief Destructor. */
-      virtual ~PointCloud () {}
+      virtual ~PointCloud () = default;


This one can also be removed, same reasons as all the other ones.

kunaltyagi · 2019-11-04T10:19:41Z

I meant to review this PR

Sorry. I didn't realize 😅 . We can revert and continue working on this. Or a new one?

SergioRAgostinho · 2019-11-04T10:39:50Z

Just a new one, assuming of course you agree with the items I pointed out.

kunaltyagi · 2019-11-05T07:25:39Z

Unneeded virtual dtors should be removed. It's good to not pay for what we don't use.

kunaltyagi requested changes Oct 25, 2019

View reviewed changes

SergioRAgostinho added the module: common label Oct 25, 2019

SergioRAgostinho self-assigned this Oct 25, 2019

kunaltyagi reviewed Oct 25, 2019

View reviewed changes

common/include/pcl/correspondence.h Show resolved Hide resolved

lightyear15 mentioned this pull request Oct 30, 2019

PCL_COMMON improve readability, modernize the code #3424

Closed

kunaltyagi approved these changes Nov 1, 2019

View reviewed changes

taketwo approved these changes Nov 1, 2019

View reviewed changes

kunaltyagi merged commit 34d543d into PointCloudLibrary:master Nov 4, 2019

SergioRAgostinho reviewed Nov 4, 2019

View reviewed changes

lightyear15 deleted the common_removeDefCtr branch November 6, 2019 12:39

lightyear15 mentioned this pull request Nov 6, 2019

PCL_COMMON: removing default destructors - part 2 #3454

Merged

dlazenby mentioned this pull request May 4, 2020

[common/point_cloud] Please re-add virtual destructor to PointCloud for safe inheritance #4036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PCL_COMMON: removing default constructors/destructors #3440

PCL_COMMON: removing default constructors/destructors #3440

lightyear15 commented Oct 24, 2019

kunaltyagi commented Oct 24, 2019

kunaltyagi left a comment

kunaltyagi Oct 25, 2019

lightyear15 Oct 25, 2019

kunaltyagi Oct 26, 2019

lightyear15 Oct 26, 2019

kunaltyagi Nov 1, 2019

lightyear15 Nov 2, 2019

kunaltyagi Nov 4, 2019

lightyear15 commented Oct 25, 2019

kunaltyagi commented Oct 26, 2019

lightyear15 commented Oct 26, 2019

kunaltyagi commented Oct 30, 2019 •

edited

lightyear15 commented Oct 30, 2019

kunaltyagi commented Oct 30, 2019

kunaltyagi left a comment •

edited

SergioRAgostinho left a comment

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

SergioRAgostinho Nov 4, 2019

taketwo Nov 4, 2019

SergioRAgostinho Nov 4, 2019

taketwo Nov 4, 2019

SergioRAgostinho Nov 4, 2019

kunaltyagi commented Nov 4, 2019

SergioRAgostinho commented Nov 4, 2019

kunaltyagi commented Nov 5, 2019

		@@ -65,6 +65,7 @@ namespace pcl

		/** \brief Destructor. */
		virtual ~StopWatch () = default;

PCL_COMMON: removing default constructors/destructors #3440

PCL_COMMON: removing default constructors/destructors #3440

Conversation

lightyear15 commented Oct 24, 2019

kunaltyagi commented Oct 24, 2019

kunaltyagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightyear15 commented Oct 25, 2019

kunaltyagi commented Oct 26, 2019

lightyear15 commented Oct 26, 2019

kunaltyagi commented Oct 30, 2019 • edited

lightyear15 commented Oct 30, 2019

kunaltyagi commented Oct 30, 2019

kunaltyagi left a comment • edited

Choose a reason for hiding this comment

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunaltyagi commented Nov 4, 2019

SergioRAgostinho commented Nov 4, 2019

kunaltyagi commented Nov 5, 2019

kunaltyagi commented Oct 30, 2019 •

edited

kunaltyagi left a comment •

edited