Modernized code with clang-tidy [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

[kunaltyagi]

Could I request you to help integrate clang-tidy in the CI before adding the improvements to prevent a backslide later on?

[gnawme]

Could I request you to help integrate clang-tidy in the CI before adding the improvements to prevent a backslide later on?

@kunaltyagi If there's a way to run clang-tidy as a last-stage check before merging, sure.

As something to run with each CI build, however, it makes CI much longer without adding much value. Policy at places I've worked is for the developer to run clang-tidy when they're getting close to being ready to merge.

[kunaltyagi]

Does it take long to run clang-tidy? (How long did it take to run on PCL?)

[gnawme]

Does it take long to run clang-tidy? (How long did it take to run on PCL?)

Try this:

• Add set(CMAKE_EXPORT_COMPILE_COMMANDS ON) to CMakeLists.txt
• Run cmake .
• Make sure compile_commands.json is present.
• Assuming you have clang-tidy installed, run (for example) run-clang-tidy -header-filter='.*' -checks=".*modernize-use-nullptr" -fix

[kunaltyagi]

clang-tidy on my system takes 20 minutes, with a full compile (including cuda modules) requiring 45 minutes.

@PointCloudLibrary/maintainers What about trying GitHub actions for this?

[SergioRAgostinho]

No opposition from my side.

[kunaltyagi]

@gnawme Could you please add a clang-tidy action to this PR on the lines of the format CI:

• clone in a docker
• run clang-format
• diff to see if there's a regression
• show diff else pass

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions

[gnawme]

@gnawme Could you please add a clang-tidy action to this PR on the lines of the format CI:

• clone in a docker
• run clang-format
• diff to see if there's a regression
• show diff else pass

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions

@kunaltyagi Add this sequence to formatting.yaml? OK, I'll take a look.

[gnawme]

@gnawme Could you please add a clang-tidy action to this PR on the lines of the format CI:

• clone in a docker
• run clang-format
• diff to see if there's a regression
• show diff else pass

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions

@kunaltyagi Add this sequence to formatting.yaml? OK, I'll take a look.

This will be in .ci/azure-pipelines/tidying.yaml; how do I test this?

jobs:
  - job: tidying
    displayName: Modernize C++
    pool:
      vmImage: 'ubuntu-latest'
    container: tdy
    steps:
      - checkout: self
        # find the commit hash on a quick non-forced update too
        fetchDepth: 10
      - script: run-clang-tidy -header-filter='.*' -checks='-*,modernize-deprecated-headers,modernize-redundant-void-arg,modernize-replace-random-shuffle,modernize-use-equals-default,modernize-use-equals-delete,modernize-use-nullptr,modernize-use-override,modernize-use-using' -fix
        displayName: 'Run clang-tidy'
      - script: git diff > formatting.patch
        displayName: 'Compute diff'
      - script: cat formatting.patch
        displayName: 'Show diff'
      - task: CopyFiles@2
        inputs:
          sourceFolder: '$(Build.SourcesDirectory)'
          contents: 'formatting.patch'
          targetFolder: '$(Build.ArtifactStagingDirectory)'
        displayName: 'Copy diff to staging directory'
      - task: PublishBuildArtifacts@1
        inputs:
          pathtoPublish: '$(Build.ArtifactStagingDirectory)'
          artifactName: formatting
        displayName: 'Publish diff'
      - script: "[ ! -s formatting.patch ]"
        displayName: 'Set job exit status'

[kunaltyagi]

how do I test this?

I don't know. Does the file needs to be in master for this? @shrijitsingh99

[shrijitsingh99]

how do I test this?

Just setup Github Actions on your fork. That should work. Nothing to be done on PCL side to test.

Could I request you to help integrate clang-tidy in the CI before adding the improvements to prevent a backslide later on?

I would also recommend looking into #4004. The idea was to first integrate it into CMake or provide some tooling for running clang-tidy then integrate it. As I remember the blocker was CMake version since newer versions had tidy support.

Without providing tooling for contributors to run clang-tidy easily, adding a check to CI won't be that useful IMO.

[gnawme]

I would also recommend looking into #4004. The idea was to first integrate it into CMake or provide some tooling for running clang-tidy then integrate it. As I remember the blocker was CMake version since newer versions had tidy support.

Older CMake versions are also blocking the transition to C++17 or later, since they don't recognize CMAKE_CXX_STANDARD 17. Are there plans to remedy that going forward?

Without providing tooling for contributors to run clang-tidy easily, adding a check to CI won't be that useful IMO.

Yes, clang-tidy was left as a developer task at places I worked that used clang-tidy. (There was a bash script in the repo's .ci directory that wasn't actually run by CI.)

@kunaltyagi I recommend that this initial modernization be a one-time task, to be revisited periodically. Backsliding will just have to be caught by reviewers.

[shrijitsingh99]

Are there plans to remedy that going forward?

Have opened an issue #4004, we can continue the discussion there.

Yes, clang-tidy was left as a developer task at places I worked that used clang-tidy. (There was a bash script in the repo's .ci directory that wasn't actually run by CI.)

Yeah, for now at least we can also provide a bash script. Maybe using the commands you are currently running for clang-tidy.

[gnawme]

@kunaltyagi once these changes are approved, I think the affected files could use a clang-format pass.

[kunaltyagi]

I think the affected files could use a clang-format pass.

We've been rolling out clang-format bit by bit (since the patch can be monstrous as well has mess up the comments and some code). Which module do you think should be prioritised? PR by modifying the whitelist in .dev and running format 2 times would suffice though it might not get reviewed soon (GSoC pressure)

[gnawme]

I think the affected files could use a clang-format pass.

We've been rolling out clang-format bit by bit (since the patch can be monstrous as well has mess up the comments and some code). Which module do you think should be prioritised? PR by modifying the whitelist in .dev and running format 2 times would suffice though it might not get reviewed soon (GSoC pressure)

These are the directories where the clang-tidy changes occurred in 1b90309b9:

• common (2307)
• features (6323)
• filters (3102)
• geometry (142)
• io (11764)
• kdtree (595)
• keypoints (4043)
• ml (5558)
• octree (2316)
• outofcore (2838)
• people (871)
• recognition (11812)
• registration (8697)
• sample_consensus (4882)
• search (1320)
• segmentation (7727)
• stereo (1937)
• surface (7418)
• tools (789)
• tracking (150)
• visualization (6600)

Which will give most bang for the buck if they had clang-format run on them?

[kunaltyagi]

Lines of code affected in each module would be a better indicator. You can get per directory stats (in alphabetical order) using:

ls -d ./* | xargs -n1 bash -c 'git diff --numstat master -- $@ | python -c "import fileinput as f; print(sum([int(line.strip()[0]) for line in f.input()]))"' {}

The largest one should be the priority

[SergioRAgostinho]

I would say following dependency graph should be top priority. The changes you make in the "base modules" are more likely to affect more parts of the code.

Edit: Here's the dep graph, for reference. Ignore the colors

[gnawme]

I would say following dependency graph should be top priority. The changes you make in the "base modules" are more likely to affect more parts of the code.

@SergioRAgostinho Wow, your graph is a lot cleaner and simpler than the one produced by cmake --graphviz=graph. What did you use to produce it?

[gnawme]

@kunaltyagi @SergioRAgostinho I ran clang-format on common, kdtree, and octree, net 1281 LOC changed (including clang-tidy changes). Shall I run clang-format on another module or two?

[SergioRAgostinho]

I would say following dependency graph should be top priority. The changes you make in the "base modules" are more likely to affect more parts of the code.

@SergioRAgostinho Wow, your graph is a lot cleaner and simpler than the one produced by cmake --graphviz=graph. What did you use to produce it?

I didn't, @divmadan did :)

@kunaltyagi @SergioRAgostinho I ran clang-format on common, kdtree, and octree, net 1281 LOC changed (including clang-tidy changes). Shall I run clang-format on another module or two?

My preferred approach is to submit a PR per tool per module e.g. pass cmake tidy over common open PR, once merged enable cmake format for common, continue for other modules. We just had a PR of equivalent magnitude in LoC changes, a "simple find replace", and it was a nightmare to review.

[SergioRAgostinho]

Split the PR.

[gnawme]

Split the PR.

OK, opened #4283 to deal with empty destructors, reverted this PR to focus on the clang-tidy changes.

[kunaltyagi]

I think there's a header-filter regex

[stale]

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

[gnawme]

Replaced by #4560, which is taking a phased approach to applying clang-tidy