Use clang_tidy instead of ament_clang_tidy

[ivanpauno]

Related to #132.

Summary

Uses clang-tidy instead of ament_clang_tidy.
ament_clang_tidy does not really pass all source files to clang-tidy, but some based on the compile commands.
So I had to fix many warnings.

Something to discuss for a follow-up: clang-tidy still needs to be run inside the docker container because:

1. The compile commands need to be generated inside it.
2. It needs the dependencies installed in order to understand the code.
3. It needs clang-tidy installed.

That can be solved by making the hook show a warning instead of failing when the compile commands cannot be generated or clang-tidy is not available.
e.g. using pre-commit/pre-commit#923 (comment).
For that, it would maybe be better to use a repo local entry instead of https://github.com/pocc/pre-commit-hooks, which isn't really doing much as it uses the locally installed clang-tidy.
On the other hand https://github.com/pre-commit/mirrors-clang-format provides much more, as it uses a python wheel that you can pin a clang-format version (and not the local one).

Other solutions:

• Do not run clang-tidy with pre-commit, but have an special CI step for that (with a script that can be run locally).

Checklist

• Read the contributing guidelines.
• Configured pre-commit and ran colcon test locally.
• Signed all commits for DCO.
• Added tests (regression tests for bugs, coverage of new code for features).
• Updated documentation (as needed).
• Checked that CI is passing.

[ivanpauno]

BTW, I'm planning to migrate beluga_amcl linters in a different PR.

[ivanpauno]

I have to double check why I didn't see those errors locally ....

[nahueespinosa]

@ivanpauno Left a few comments!

I'm guessing that ament_clang_tidy was ignoring the test directory, so it wasn't instantiating some templates either.

[ivanpauno]

@ivanpauno Left a few comments!

I'm guessing that ament_clang_tidy was ignoring the test directory, so it wasn't instantiating some templates either.

Exactly, https://github.com/ament/ament_lint/blob/a1b6c57be91b05ffa11d5eb6ff8700a478f1a890/ament_clang_tidy/ament_clang_tidy/main.py#L188-L189.
I don't know why they decided to do that, it doesn't seem like a good idea IMHO.

[nahueespinosa]

LGTM!

[nahueespinosa]

clang-tidy still needs to be run inside the docker container

That can be solved by making the hook show a warning instead of failing when the compile commands cannot be generated or clang-tidy is not available.

That sounds like the way to go. It'd be neat to disambiguate between "couldn't run because dependencies are missing" and "there are issues with the code", to generate a warning or an error accordingly.

[ivanpauno]

That sounds like the way to go. It'd be neat to disambiguate between "couldn't run because dependencies are missing" and "there are issues with the code", to generate a warning or an error accordingly.

I think it would be possible to do something like that if we have our own implementation of the pre-commit hook wrapper, but I don't think that's available in the existing wrapper.
It should be more or less easy to do though, so I will take a look to it at some point.