[registration] Remove deprecated method Registration::setInputCloud() from public API

[kunaltyagi]

Fixes #2724 by making compiler emit compile error if previously deprecated method is used

[kunaltyagi]

I'm not sure that this doesn't break behavior of downstream users in subtle ways.

[taketwo]

I only see a potential of this breaking downstream project in a non-subtle way :)

[SergioRAgostinho]

Merge at your convenience if the CI is green.

[taketwo]

According to this, deleting a virtual function in deriving classes is not allowed by the standard. Changing visibility (as originally proposed by @kunaltyagi) is one of the listed workarounds. Another one is to add a static_assert to the override. Perhaps it would be a more user-friendly approach because we can explain the user what to do in the assertion message.

[kunaltyagi]

I tested out a snippet in Godbolt and it didn't throw an error. Please show me the error in my ways 😆. A valid snippet would be very welcome indeed.

[SergioRAgostinho]

Your example was broken. You cannot have a templated virtual function, so the function you declared was not overriding the original one. See the modified snippet https://godbolt.org/z/LdkanP

[kunaltyagi]

static_assert(false,...) will fail immediately. The template is there to delay it from being evaluated by the compiler

See snippet

[SergioRAgostinho]

I took another look at the SO thread and the associated comments and they flag that the method can still be called from the base class, with the static_assert option. This happens because we're redefining the method in the derived class, not really overriding it.

Both solutions on the table (static assert and moving th private) suffer from the same, there's no way to prevent the call from a base pointer, because the base pointer has the method implemented.

Given the circumstances I vote to move it to private but keep the original implementation we deleted. There's no point in having a static deprecated warning because we'll never be able to delete the method properly, so we should just throw that warning at runtime.

Then again, if a user is gonna keep around a vector of base class pointers, I believe logically we would store Registration pointers, to invoke different registration methods. Not really PCLBase, because it has no functional value.

[kunaltyagi]

PTAL. Updated to use private + runtime warning.

[SergioRAgostinho]

if CI == green: 🚀