Modernize code to use override

[SunBlack]

Changes are done by: run-clang-tidy -header-filter='.*' -checks='-*,modernize-use-override' -fix

[taketwo]

Nice! Please rebase on the latest master, hopefully the Ubuntu job will not fail anymore.

[taketwo]

Please don't mark destructors of derived classes as override (ref).

[SunBlack]

Ok, I will remove them from destructors. I currently just wait for another run of clang-tidy, because last run was without CUDA. I hope I have now all projects enabled (except optronic_viewer #2701)

[SunBlack]

Integrated new run of clang-tidy and reverted changes to destructors.

//Edit: Changes to destructors done via Regex (~.*) override => $1

[jasjuang]

@taketwo I looked into the link and it doesn't really explain why we shouldn't mark destructors of derived classes as override. Can you explain why? I marked destructors of derived classes as override for all of my projects...

[taketwo]

@jasjuang here is a related discussion: isocpp/CppCoreGuidelines#721. There are arguments in both directions, but in the end I'd stick with what the Core Guidelines recommend.

[taketwo]

All looks good. Though I'd ask you to revert changes to the 3rdparty code that is bundled with PCL:

• recognition/include/pcl/recognition/3rdparty/metslib
• surface/include/pcl/surface/3rdparty/opennurbs
• surface/src/3rdparty/opennurbs/

[SunBlack]

Created a new commit with your requested changes included, because after last rebase GitHub displayed a wrong overview which files were changed (showed diff regarding changed whitespaces in CMakeLists of my last PR merged into master).

[jasjuang]

@taketwo I spent some time going through the discussions I still don't see why we shouldn't mark destructors of the derived classes as override. It prevents bug like someone forgetting to make the destructors of the base class virtual, and the only argument against it seems to be the override in the destructors isn't really overriding in the usual sense for normal functions which I think is way less important than prevent the forgot to mark base class destructor virtual bug.

[SunBlack]

It prevents bug like someone forgetting to make the destructors of the base class virtual

There exists a compiler warning (-Wdelete-non-virtual-dtor) for this, so after #2746 this is no issue.

In general: I don't have really a opinion about this. So it is for me a decision of the maintainers of this project. Just in case there will be added an additional check with clang-tidy on Azure we should add override to destructors, so we can check for missing overrides in non-destructor methods.

//Edit: -Wdelete-non-virtual-dtor is added in Clang 6.0. GCC seems not to support this. But we build servers are only running Clang 4.0 (Mac build is running AppleClang 9.1, which is Clang 4.0, see here). So we should add maybe a second Mac build with newer Clang or Ubuntu 18.04, where Clang 6.0 is default Clang version.

[taketwo]

@jasjuang I don't have a very strong opinion here. I just thought it's a good idea to follow the guidelines endorsed by the C++ standardization committee. I agree with you though that there is value in marking them as override.

@SunBlack Adding Ubuntu 18.04 based build is somewhere on my TODO list. And yes, for variety, we can use Clang there.