Remove redundant member field initialization #3070

SunBlack · 2019-05-07T19:52:28Z

Changes are done by run-clang-tidy -header-filter='.*' -checks='-*,readability-redundant-member-init' -fix

I sure there are more unnecessary explicit member init (and unnecessary empty constructors), but PR is already big enough with these automatic changes.

…adability-redundant-member-init' -fix

SergioRAgostinho

Gave up on reviewing. 700 touched lines of code.

Edit: why not follow the usual strategy of splitting over modules?

SergioRAgostinho · 2019-05-08T07:07:18Z

common/include/pcl/common/synchronizer.h

@@ -64,7 +64,7 @@ namespace pcl
    int callback_counter;
  public:

-    Synchronizer () : mutex1_ (), mutex2_ (), publish_mutex_ (), queueT1 (), queueT2 (), cb_ (), callback_counter (0) { };


Any idea why clang "ignored" the other 3 default initialized members?

SergioRAgostinho · 2019-05-08T07:07:57Z

common/include/pcl/impl/pcl_base.hpp

@@ -45,7 +45,6 @@
 template <typename PointT>
 pcl::PCLBase<PointT>::PCLBase ()
  : input_ ()
-  , indices_ ()


Same question here for input().

SergioRAgostinho · 2019-05-08T07:09:22Z

features/include/pcl/features/3dsc.h

@@ -104,7 +104,6 @@ namespace pcl
        min_radius_(0.1),
        point_density_radius_(0.2),
        descriptor_length_ (),
-        rng_ (),


descriptor_length_()

SergioRAgostinho · 2019-05-08T07:09:47Z

features/include/pcl/features/esf.h

@@ -74,7 +74,7 @@ namespace pcl
      typedef typename Feature<PointInT, PointOutT>::PointCloudOut PointCloudOut;

      /** \brief Empty constructor. */
-      ESFEstimation () : lut_ (), local_cloud_ ()
+      ESFEstimation () : local_cloud_ ()


Same comment here.

SergioRAgostinho · 2019-05-08T07:10:18Z

features/include/pcl/features/impl/moment_of_inertia_estimation.hpp

  aabb_min_point_ (),
  aabb_max_point_ (),
  obb_min_point_ (),
  obb_max_point_ (),
-  obb_position_ (0.0f, 0.0f, 0.0f),


Same comment here.

SergioRAgostinho · 2019-05-08T08:02:19Z

surface/include/pcl/surface/grid_projection.h

@@ -85,7 +85,7 @@ namespace pcl
      /** \brief Data leaf. */
      struct Leaf
      {
-        Leaf () : data_indices (), pt_on_surface (), vect_at_grid_pt () {}
+        Leaf () {}


make it default.

SergioRAgostinho · 2019-05-08T08:02:27Z

surface/include/pcl/surface/impl/grid_projection.hpp

-  cell_hash_map_ (), min_p_ (), max_p_ (), leaf_size_ (0.001), gaussian_scale_ (),
-  data_size_ (0), max_binary_search_level_ (10), k_ (50), padding_size_ (3), data_ (),
-  vector_at_data_point_ (), surface_ (), occupied_cell_list_ ()
+  cell_hash_map_ (), leaf_size_ (0.001), gaussian_scale_ (),


Same comment here.

SergioRAgostinho · 2019-05-08T08:02:46Z

surface/include/pcl/surface/impl/grid_projection.hpp

-  data_size_ (0), max_binary_search_level_ (10), k_ (50), padding_size_ (3), data_ (),
-  vector_at_data_point_ (), surface_ (), occupied_cell_list_ ()
+  cell_hash_map_ (), leaf_size_ (resolution), gaussian_scale_ (),
+  data_size_ (0), max_binary_search_level_ (10), k_ (50), padding_size_ (3), data_ () 


Same comment here.

SergioRAgostinho · 2019-05-08T08:02:59Z

surface/include/pcl/surface/impl/mls.hpp

@@ -838,7 +838,7 @@ template <typename PointInT, typename PointOutT>
 pcl::MovingLeastSquares<PointInT, PointOutT>::MLSVoxelGrid::MLSVoxelGrid (PointCloudInConstPtr& cloud,
                                                                          IndicesPtr &indices,
                                                                          float voxel_size) :
-  voxel_grid_ (), bounding_min_ (), bounding_max_ (), data_size_ (), voxel_size_ (voxel_size)
+  voxel_grid_ (), data_size_ (), voxel_size_ (voxel_size)


Same comment here.

SergioRAgostinho · 2019-05-08T08:03:07Z

surface/include/pcl/surface/mls.h

@@ -295,9 +295,7 @@ namespace pcl

      /** \brief Empty constructor. */
      MovingLeastSquares () : CloudSurfaceProcessing<PointInT, PointOutT> (),
-                              normals_ (),
                              distinct_cloud_ (),


Same comment here.

taketwo · 2019-05-08T08:35:28Z

Edit: why not follow the usual strategy of splitting over modules?

👍 though I'd say there is no need for a separate PR for each and every module. This overloads our 10 free runners on CI. Last time it took over a day to run all the jobs through. We may group modules with few changes into a single PR.

Edit: @SergioRAgostinho did you take a note of which modules' changes you reviewer? I propose to limit this PR to those modules and strip everything else.

SergioRAgostinho · 2019-05-08T09:06:19Z

All files in alphabetical order up to the last comment. So it means I stopped halfway through surface.

taketwo

I wanted to go through the rest, but realized to be consistent with your suggestions, it's better if we formulate a "meta-suggestion" (what we are looking to have in general) first. As far as I understand:

Check remaining default-constructed members, eliminate those that are not needed;
Eleminate (or = default) constructors that have not init-list and empty body.

Anything else I missed?

taketwo · 2019-05-08T09:22:25Z

surface/include/pcl/surface/processing.h

@@ -99,7 +99,7 @@ namespace pcl
      typedef PolygonMesh::ConstPtr PolygonMeshConstPtr;

      /** \brief Constructor. */
-      MeshProcessing () : input_mesh_ () {}
+      MeshProcessing () {}


Suggested change

MeshProcessing () {}

MeshProcessing () = default;

taketwo · 2019-05-08T09:27:02Z

common/include/pcl/console/time.h

@@ -50,7 +50,7 @@ namespace pcl
    {
      public:

-        TicToc () : tictic_ () {}
+        TicToc () {}


No need in explicit constructor.

SunBlack · 2019-05-08T09:59:25Z

Edit: why not follow the usual strategy of splitting over modules?

I knew I will open another can of worms with this. So I wanted to force you to only check automatic changes of clang-tidy. If we really want to clean constructors, this will be thousand of lines, because then we have to think about constructor inheritance, sense of some classes, like here:

pcl/apps/in_hand_scanner/include/pcl/apps/in_hand_scanner/in_hand_scanner.h

Lines 215 to 229 in 1d1429a

    
           /** \brief Helper object for the computation thread. Please have a look at the documentation of calcFPS. */ 
        
           class ComputationFPS : public Base::FPS 
        
           { 
        
             public: 
        
               ComputationFPS () {} 
        
               ~ComputationFPS () {} 
        
           }; 
        
           /** \brief Helper object for the visualization thread. Please have a look at the documentation of calcFPS. */ 
        
           class VisualizationFPS : public Base::FPS 
        
           { 
        
             public: 
        
               VisualizationFPS () {} 
        
               ~VisualizationFPS () {} 
        
           };

And of course we have to check a lot of documentary seen (e.g. clang-tidy didn't removed constructor of std::vector in case there is a custom allocator (Eigen))

In general I suggest to check here only clang-tidy fixes and make separate PR for manual search of unnecessary code.

SergioRAgostinho · 2019-05-08T10:46:49Z

because then we have to think about constructor inheritance, sense of some classes, like here:

pcl/apps/in_hand_scanner/include/pcl/apps/in_hand_scanner/in_hand_scanner.h

Lines 215 to 229 in 1d1429a

/** \brief Helper object for the computation thread. Please have a look at the documentation of calcFPS. */

class ComputationFPS : public Base::FPS

{

public:

ComputationFPS () {}

~ComputationFPS () {}

};

/** \brief Helper object for the visualization thread. Please have a look at the documentation of calcFPS. */

class VisualizationFPS : public Base::FPS

{

public:

VisualizationFPS () {}

~VisualizationFPS () {}

};

🤦‍♂

And of course we have to check a lot of documentary seen (e.g. clang-tidy didn't removed constructor of std::vector in case there is a custom allocator (Eigen))

Any idea why that happens?

In general I suggest to check here only clang-tidy fixes and make separate PR for manual search of unnecessary code.

I see. In that case I say we simply merge as is. This was an automated change, it is compiling and passing all tests.

taketwo · 2019-05-08T14:03:05Z

My terminal is swamped with:

../visualization/include/pcl/visualization/pcl_visualizer.h:2041:11: warning: base class ‘class vtkCommand’ should be explicitly initialized in the copy constructor [-Wextra]
           FPSCallback (const FPSCallback& src) : actor (src.actor), pcl_visualizer (src.pcl_visualizer), decimated (src.decimated), last_fps (src.last_fps) {}
           ^~~~~~~~~~~

after merging this. Will send a fix reintroducing vtkCommand() to initializer list soon.

SunBlack · 2019-05-08T14:19:04Z

You are right, the ubuntu build is flooded by this (500 warnings of this type). As far as I can see both are right: clang-tidy & compiler.

Example:

Before:

pcl/octree/include/pcl/octree/octree_container.h

Lines 141 to 145 in 99362da

    
           /** \brief Empty constructor. */ 
        
           OctreeContainerEmpty (const OctreeContainerEmpty&) : 
        
               OctreeContainerBase () 
        
           { 
        
           }

Now:

pcl/octree/include/pcl/octree/octree_container.h

Lines 140 to 143 in 6c3bbea

    
           /** \brief Empty constructor. */ 
        
           OctreeContainerEmpty (const OctreeContainerEmpty&) 
        
           { 
        
           }

Correct:

      OctreeContainerEmpty (const OctreeContainerEmpty& other) :
          OctreeContainerBase (other)
      {
      }

clang-tidy removed it, because default base constructor will be always executed. -Wextra warns, because the base copy constructor should be called and not the default constructor.

Or just remove custom copy constructor implementation. I think this would be the best solution.

SunBlack · 2019-05-08T14:56:55Z

Just mention: I currently going throw the warnings. Not sure if I get ready with my remaining time today. If not I send a PR with already fixed warnings,

taketwo · 2019-05-08T14:58:54Z

Just mention: I currently going throw the warnings. Not sure if I get ready with my remaining time today. If not I send a PR with already fixed warnings,

I've already submitted #3075. But I may have missed some, of course.

SunBlack · 2019-05-08T15:06:57Z

I've already submitted #3075. But I may have missed some, of course.

I filtered output of Azure and yeah, you missed a lot ;).

SergioRAgostinho · 2019-05-08T15:34:47Z

Or just remove custom copy constructor implementation. I think this would be the best solution.

I vote to remove it.

Will send a fix reintroducing vtkCommand() to initializer list soon.

Don´t forget to add a comment on the side so we don't feel tempted to accidentally remove it again.

taketwo · 2019-05-08T15:39:28Z

Don´t forget to add a comment on the side so we don't feel tempted to accidentally remove it again.

In the process we figured out (see comments in the linked PR) that it should be vtkCommand (src), not just vtkCommand (). Thus clang-tidy won't be tempted to remove it again anyway.

Changes are done by run-clang-tidy -header-filter='.*' -checks='-*,re…

1d1429a

…adability-redundant-member-init' -fix

SunBlack force-pushed the readability-redundant-member-init branch from 7819abc to 1d1429a Compare May 7, 2019 19:52

SunBlack mentioned this pull request May 7, 2019

[Overview] Modernization code with clang-tidy #2731

Open

SergioRAgostinho reviewed May 8, 2019

View reviewed changes

taketwo reviewed May 8, 2019

View reviewed changes

taketwo merged commit 6c3bbea into PointCloudLibrary:master May 8, 2019

SunBlack deleted the readability-redundant-member-init branch May 8, 2019 13:29

taketwo mentioned this pull request May 8, 2019

Fix warnings reported by GCC #3075

Merged

taketwo added the modernization label Jan 18, 2020

taketwo changed the title ~~Remove redundant member init~~ Remove redundant member field initialization Jan 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant member field initialization #3070

Remove redundant member field initialization #3070

SunBlack commented May 7, 2019

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

SergioRAgostinho May 8, 2019

taketwo commented May 8, 2019 •

edited

Loading

SergioRAgostinho commented May 8, 2019

taketwo left a comment

taketwo May 8, 2019

taketwo May 8, 2019

SunBlack commented May 8, 2019 •

edited

Loading

SergioRAgostinho commented May 8, 2019

taketwo commented May 8, 2019

SunBlack commented May 8, 2019 •

edited

Loading

SunBlack commented May 8, 2019

taketwo commented May 8, 2019

SunBlack commented May 8, 2019

SergioRAgostinho commented May 8, 2019

taketwo commented May 8, 2019

Remove redundant member field initialization #3070

Remove redundant member field initialization #3070

Conversation

SunBlack commented May 7, 2019

SergioRAgostinho left a comment • edited Loading

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

taketwo commented May 8, 2019 • edited Loading

SergioRAgostinho commented May 8, 2019

taketwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack commented May 8, 2019 • edited Loading

SergioRAgostinho commented May 8, 2019

taketwo commented May 8, 2019

SunBlack commented May 8, 2019 • edited Loading

SunBlack commented May 8, 2019

taketwo commented May 8, 2019

SunBlack commented May 8, 2019

SergioRAgostinho commented May 8, 2019

taketwo commented May 8, 2019

SergioRAgostinho left a comment •

edited

Loading

taketwo commented May 8, 2019 •

edited

Loading

SunBlack commented May 8, 2019 •

edited

Loading

SunBlack commented May 8, 2019 •

edited

Loading