Modernize with clang-tidy from base subprojects [PCL-2731]

[gnawme]

Initial modernization of code base using clang-tidy and an innocuous subset of its modernize checks:

• modernize-avoid-bind
• modernize-avoid-c-arrays
• modernize-concat-nested-namespaces
• modernize-deprecated-headers
• modernize-deprecated-ios-base-aliases
• modernize-loop-convert
• modernize-make-shared
• modernize-make-unique
• modernize-pass-by-value
• modernize-raw-string-literal
• modernize-redundant-void-arg
• modernize-replace-auto-ptr
• modernize-replace-random-shuffle
• modernize-return-braced-init-list
• modernize-shrink-to-fit
• modernize-unary-static-assert
• modernize-use-auto
• modernize-use-bool-literals
• modernize-use-default-member-init
• modernize-use-emplace
• modernize-use-equals-default
• modernize-use-equals-delete
• modernize-use-nodiscard
• modernize-use-noexcept
• modernize-use-nullptr
• modernize-use-override
• modernize-use-trailing-return-type
• modernize-use-transparent-functors
• modernize-use-uncaught-exceptions
• modernize-use-using

This PR supersedes #4249, which was much too large for sensible review.

It takes the approach of starting at the base of the PCL dependency graph, and only applying clang-tidy in those subprojects.

https://user-images.githubusercontent.com/1709142/87282880-fd5cc080-c4f4-11ea-878e-9c7de9c71eb8.png

[gnawme]

@kunaltyagi Per @SergioRAgostinho I replaced #4249 with the first of what will be a series of much smaller PRs to introduce clang-tidy improvements into the code base.

[gnawme]

@kunaltyagi CI is green for this PR as well

[Morwenn]

Shoudn't CMAKE_EXPORT_COMPILE_COMMANDS rather be passed as a -Dcommand-line option to CMake in tidy-modern.yml? Adding them to a bunch of CMakeLists.txt seems both redundant and not something you want to do most of the time.

[gnawme]

Shoudn't CMAKE_EXPORT_COMPILE_COMMANDS rather be passed as a -Dcommand-line option to CMake in tidy-modern.yml? Adding them to a bunch of CMakeLists.txt seems both redundant and not something you want to do most of the time.

@Morwenn That was done intentionally to limit the scope of clang-tidy changes to those projects where the CMakeLists.txt was modified.

For contrast, see #4249 where CMAKE_EXPORT_COMPILE_COMMANDS was put into the top-level CMakeLists.txt. Nobody wanted to review that PR.

[Morwenn]

Oh, my bad then.

[larshg]

The addition of the tidy-modern.yml will it make a check like the format checker on the CI's? and stop the rest of the builds to continue or where will it be run/added/shown?

[gnawme]

The addition of the tidy-modern.yml will it make a check like the format checker on the CI's? and stop the rest of the builds to continue or where will it be run/added/shown?

That's the intent; @kunaltyagi wanted to prevent back-sliding in code that's already been tidied. You can see some of the discussion in #4249

[larshg]

It looks fine for me. I'm just wondering, if this action doesn't run succesfully for a PR, would it then "block" the PR?
Wonder if its ends up as a check?

There is however some difference for the I/O module on latest run on your branch @gnawme. In https://github.com/gnawme/pcl/actions/runs/451637712

--- a/io/src/image_depth.cpp
+++ b/io/src/image_depth.cpp
@@ -66,7 +66,7 @@ pcl::io::DepthImage::DepthImage (FrameWrapper::Ptr depth_metadata, float baselin
 
 
 pcl::io::DepthImage::~DepthImage ()
-{}
+= default;
 
 

Missing those?

[gnawme]

It looks fine for me. I'm just wondering, if this action doesn't run succesfully for a PR, would it then "block" the PR?
Wonder if its ends up as a check?

There is however some difference for the I/O module on latest run on your branch @gnawme. In https://github.com/gnawme/pcl/actions/runs/451637712

--- a/io/src/image_depth.cpp
+++ b/io/src/image_depth.cpp
@@ -66,7 +66,7 @@ pcl::io::DepthImage::DepthImage (FrameWrapper::Ptr depth_metadata, float baselin
 
 
 pcl::io::DepthImage::~DepthImage ()
-{}
+= default;
 
 

Missing those?

@larshg Yes, @SergioRAgostinho asked me to remove unnecessary default constructs/destructors in the previous PR, perhaps I can do that when I add clang-tidy to the next subproject(s)?

[kunaltyagi]

Code changes LGTM. Just a few questions on the surrounding infra.

Apologies, but I had to be away for a while, and have lost the original context of the PR

[SunBlack]

Just note: Here was the decision not to add override to destructors.

[kunaltyagi]

That's true. But the CppCoreGuidelines reaffirmed their position isocpp/CppCoreGuidelines#1448 (in 2019, that discussion is from 2018, and @taketwo says the Core Guidelines position is prefered). The new PR says: even dtors should have one of virtual....

override makes sense to catch errors in dtors (to prevent unnecessary virtual base classes). What are your thoughts on this @SunBlack ?

[SunBlack]

We are using override for dtor in our project sine years ;-). I prefer it as on this way you can be sure, that the virtual is not missing in the base class. Just wanted to mention it that there was the decision as I didn't knew CppCoreGuidelines changed their mind.

I just would prefer to have these modernizations in separate PRs like in #2731, as we can apply them to the whole project once. And this PR should be for enabling the checks on the CI. Imho we should enable this not just for some modules first. Instead for the whole project and than add fixed categories one by one (we just have to exclude 3rd party code somehow - maybe move the 3rd party code to a separate dir in the root project?)

[kunaltyagi]

In that case, override stays :)

And this PR should be for enabling the checks on the CI

To confirm, you are advising splitting this PR into 2 parts:

• switch on the github workflow for clang-tidy checking (and the relevant per-module changes to cmake)
• apply mitigations (one or several)

we just have to exclude 3rd party code somehow - maybe move the 3rd party code to a separate dir in the root project

If code reorganization is needed, it'd get quite messy. It might be better to have multiple CI workflows, each with their "exception"-list and config till we manage to bring order. If we delay clang-tidy due to code re-organization, I fear we might not end up with either of them. Moreover, the piece-meal approach will give us insight into any commonality between the workflows and help in re-organization

[SunBlack]

To confirm, you are advising splitting this PR into 2 parts:

• switch on the github workflow for clang-tidy checking (and the relevant per-module changes to cmake)
• apply mitigations (one or several)

Yep as otherwise we end like with clang-format: Some parts of the project are modernized, other not. As already most important changes are done via #2731 this should be mostly only smaller PRs now in preparation for including it in the CI.

we just have to exclude 3rd party code somehow - maybe move the 3rd party code to a separate dir in the root project

If code reorganization is needed, it'd get quite messy. It might be better to have multiple CI workflows, each with their "exception"-list and config till we manage to bring order. If we delay clang-tidy due to code re-organization, I fear we might not end up with either of them. Moreover, the piece-meal approach will give us insight into any commonality between the workflows and help in re-organization

Indeed. Maybe it works to add set(CMAKE_EXPORT_COMPILE_COMMANDS ON) in the root and et(CMAKE_EXPORT_COMPILE_COMMANDS OFF) just in the 3rd party projects? Or isn't it possible to turn off it just for some projects? I don't know how the integration on GitHub really works: In our project we run clang-tidy on all code and filter it later (yeah from the performance to exclude it before would be better, but it is easier as the Jenkins-Plugin handles it ;-) )

//Edit: Ah well, I see you don't check the messages from Clang-Tidy. Instead you are applying -fix option. This sadly don't work for all checks. Best way would be to get clang-tidy warnings, deduplicate them and than format them into a format Azure understand.

[kunaltyagi]

Let's get the low hanging fruits first :)

Could you take the lead on the first 2 parts of "get clang-tidy warnings, deduplicate them and than format them into a format Azure understand" given your experience (or if the project is open source, link the jenkins file for reference)?

@gnawme Sorry for all the chatter, I'll summarize the actionable items for you :). Based on @SunBlack's recommendation, please split the PR in two parts:

• Add empty cmake changes to all modules + clang-tidy with no checks
• Add a config + changes

[SunBlack]

Could you take the lead on the first 2 parts of "get clang-tidy warnings, deduplicate them and than format them into a format Azure understand" given your experience (or if the project is open source, link the jenkins file for reference)?

The project we are using is Jenkins Warnings Next Generation Plugin and the file it can parses from Clang-Tidy are like here. Not sure: You can tell run-clang-tidy to print files as yml - maybe they are better for converting into a format Azure can. Not sure which formats fits our need at best: "CTest, JUnit (including PHPUnit), NUnit 2, NUnit 3, Visual Studio Test (TRX), and xUnit 2"

//Edit: Found this - this could be already that what we want.

[gnawme]

Let's get the low hanging fruits first :)

Could you take the lead on the first 2 parts of "get clang-tidy warnings, deduplicate them and than format them into a format Azure understand" given your experience (or if the project is open source, link the jenkins file for reference)?

@gnawme Sorry for all the chatter, I'll summarize the actionable items for you :). Based on @SunBlack's recommendation, please split the PR in two parts:

• Add empty cmake changes to all modules + clang-tidy with no checks
• Add a config + changes

@kunaltyagi @SunBlack

• Modernized code with clang-tidy [PCL-2731] #4249 already applied clang-tidy to the entire project, but nobody wanted to review the PR; so this PR is already part of a divide-and-conquer strategy, which takes the approach of walking up the dependency graph
• To answer your question in an earlier discussion -- yes, you can enable/disable clang-tidy at the module level by adding set(CMAKE_EXPORT_COMPILE_COMMANDS ON) in the CMakeLists.txt for that module (which is the approach that this PR is taking)
• OTOH, I haven't tried having set(CMAKE_EXPORT_COMPILE_COMMANDS ON) at the root, and disabling it by adding set(CMAKE_EXPORT_COMPILE_COMMANDS OFF) at the module level

[kunaltyagi]

#4249 already applied clang-tidy to the entire project

That's not what I meant. Essentially, simply split this PR in 2 parts:

• changes purely for enabling clang-tidy
• config file for clang-tidy and changes due to addition of that file

[gnawme]

#4249 already applied clang-tidy to the entire project

That's not what I meant. Essentially, simply split this PR in 2 parts:

• changes purely for enabling clang-tidy
• config file for clang-tidy and changes due to addition of that file

@kunaltyagi OK, I removed the .github/workflows directory so CI won't be triggered, and left the clang-tidy changes in place. Hope that's what you meant.

Github workflows to be restored in the future (or perhaps replaced by what @SunBlack has in mind).

[kunaltyagi]

Kinda. But in reverse.

Could you make a PR which has a github workflow for clang-tidy with no checks?
As for this PR, expand the checks to all sub-modules (if you haven't). (And no, please keep the checks fixed, don't add more checks)

[kunaltyagi]

Should we add the following:

WarningsAsErrors: '#same content as in checks, but for errors'

[gnawme]

Kinda. But in reverse.

Could you make a PR which has a github workflow for clang-tidy with no checks?
As for this PR, expand the checks to all sub-modules (if you haven't). (And no, please keep the checks fixed, don't add more checks)

Sure; what are you trying to accomplish, exactly? #4249 has checks for the entire project; if I remove the GitHub workflows from the PR, will that accomplish your latter aim?

BTW, who approved using namespace std going back into the code?

[SunBlack]

Sure; what are you trying to accomplish, exactly? #4249 has checks for the entire project; if I remove the GitHub workflows from the PR, will that accomplish your latter aim?

You could run clang-tidy with all checks and add all checks which doesn't show an issue already to the workflow.

[mvieth]

@gnawme I believe this pull request can be closed, since pull request #4629 is now merged?

[gnawme]

@gnawme I believe this pull request can be closed, since pull request #4629 is now merged?

@mvieth Possibly; this PR went without attention for so long that I've forgotten.