Skip to content

Conversation

jmackay2
Copy link
Contributor

Calling virtual functions from a constructor is considered dangerous. This removes the call of the virtual function setInputCloud by setting the indices. This also simplifies some of the duplicate code in the constructors.

@jmackay2 jmackay2 marked this pull request as ready for review July 15, 2025 00:52
larshg
larshg previously approved these changes Sep 4, 2025
@larshg larshg added module: sample_consensus changelog: fix Meta-information for changelog generation labels Sep 4, 2025
@larshg larshg added this to the pcl-1.16.0 milestone Sep 4, 2025
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!
Maybe a dumb question, but what is the possible danger with calling setInputCloud from the constructor here?

SampleConsensusModel (const PointCloudConstPtr &cloud,
const Indices &indices,
bool random = false)
SampleConsensusModel (const PointCloudConstPtr &cloud, const Indices &indices = Indices(), const bool random = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could adding a default value for indices not create a possible ambiguity if only the cloud parameter is given? Then there would not be a way to distinguish between this ctor and SampleConsensusModel (const PointCloudConstPtr &cloud, const bool random = false), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is definitely ambiguous. This has been fixed by removing the default for indices.

@jmackay2
Copy link
Contributor Author

Sorry for the delay! Maybe a dumb question, but what is the possible danger with calling setInputCloud from the constructor here?

From my understanding, the danger is that setInputCloud is a virtual function for this class, which means it may not be available in the constructor since the SampleConsensusModel object has not been created yet. This means it is likely or possible to call the base function instead of the virtual function in this class.

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still do not see the specific danger in this case since SampleConsensusModel does not have a parent class, so as far as I can see, the only version of setInputCloud that could be called from the constructor is the one in SampleConsensusModel (as one would expect). But I guess following the general rule to not call a virtual function from a constructor is still a good idea. Thanks!

@mvieth mvieth merged commit a09c4c8 into PointCloudLibrary:master Sep 13, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: sample_consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants