Closed mitsudome-r closed 3 years ago
Current blockers
<!-- velodyne_driver/DriverNodelet -->
<param name="scan_phase" value="$(var scan_phase)"/>
<param name="sensor_timestamp" value="$(var sensor_timestamp)" />
also the cloud nodelet itself was turned into transform or convert node. Which one do we need?
<!-- velodyne_pointcloud/CloudNodelet -->
<param name="num_points_threshold" value="$(var num_points_threshold)"/>
<param name="invalid_intensity" value="$(var invalid_intensity)"/>
<param name="scan_phase" value="$(var scan_phase)"/>
<!-- gone from velodyne repo -->
<node pkg="nodelet" exec="nodelet" name="$(var manager)_fix_distortion" args="load velodyne_pointcloud/InterpolateNodelet $(var manager)">
<remap from="velodyne_points_ex" to="mirror_cropped/pointcloud_ex" />
<remap from="velodyne_points_interpolate" to="rectified/pointcloud" />
<remap from="velodyne_points_interpolate_ex" to="rectified/pointcloud_ex" />
</node>
See this document. The linters are executables wrapped by cmake packages.
pose_initializer
packageI added that to the pose_inializer
package and ran
colcon build --packages-up-to pose_initializer
colcon test --packages-select pose_initializer
colcon test-result --verbose
ament_cmake_copyright
It complains for all source files that it doesn't find a copyright notice, although there is one, because it doesn't understand /* */
.
This can be fixed by running ament_copyright --add-missing 'Autoware Foundation' apache2
(or whatever license holder is appropriate) and then deleting the existing license. That's probably not too hard to do in a batch operation as long as the existing licenses all look the same.
ament_cppcheck
It prints
- cppcheck
<<< failure message
-- run_test.py: invoking following command in '/home/nikolai.morin/t4/src/autoware/pilot.auto/localization/util/pose_initializer':
- /opt/ros/foxy/bin/ament_cppcheck --xunit-file /home/nikolai.morin/t4/build/pose_initializer/test_results/pose_initializer/cppcheck.xunit.xml --include_dirs /home/nikolai.morin/t4/src/autoware/pilot.auto/localization/util/pose_initializer/include
[include/pose_initializer/pose_initializer_core.h:33]: (error: syntaxError) Code 'classPoseInitializer:' is invalid C code. Use --std or --language to configure the language.
1 errors
-- run_test.py: return code 1
-- run_test.py: verify result file '/home/nikolai.morin/t4/build/pose_initializer/test_results/pose_initializer/cppcheck.xunit.xml'
>>>
This is not very readable – there is a lot of irrelevant stuff in there. The cause of the error is that the file is assumed to be C
and not C++
because of the file name extension. This can easily be fixed in a batch rename operation.
ament_cpplint
This one has a lot of output, so I split it into error categories:
#pragma once
I didn't find a tool to generate those automatically, so it would potentially mean a moderate amount of work for very little gain, at least where #pragma once
is already there.
#include
sThe pose_initializer_core.hpp
has the following includes:
#include <memory>
#include <string>
#include <rclcpp/rclcpp.hpp>
// more includes
On the rclcpp
line, cpplint
outputs this very misleading message: Found C system header after C++ system header. Should be: pose_initializer_core.h, c system, c++ system, other.
Here's what's wrong with that:
pose_initializer_core.h
, we're already in the header filerclcpp/rclcpp.hpp
is not a C system header. cpplint
thinks angle brackets = system header.Changing angle brackets of .h
or .hpp
includes to quotes should be not too much work, given that C system headers are rarely used in the codebase. But reordering the includes is more work.
Tells you to add missing C++ system headers. A moderate amount of work, because it is manual and probably affects most packages, but done in a few minutes per package. It doesn't bring that much of a quality improvement because it doesn't tell you everything that's used – e.g. it didn't pick up on ostream
, and if a build breaks because of this, it will be pretty obvious and easy to fix.
There are a bunch of Missing username in TODO; it should look like "// TODO(my_username): Stuff.
– a very good check in my opinion, since the code base has many // TODO
comments that do not even explain what the TODO is. When the code base transitions to AutowareAuto, it should be self-contained and not rely on access to the original authors.
Fixing that check would probably require pinging a lot of the original authors. Before that, we should clarify whether we should put GitHub usersnames, GitLab usernames, email addresses or real names in there.
I didn't see documentation on what things cpplint actually checks, maybe we need to read the source code. Several other things in there seem like a moderate amount of work.
ament_uncrustify
This had no output, because I think I already ran it on the original code.
I remember being annoyed with ament_uncrustify
before, because ament_uncrustify --reformat
doesn't fix all the things it complains about, but I can't find a specific example.
ament_xmllint
I think this pointed out important fixes in a previous package I ported, but for this one it pointed out nothing.
ament_pclint
: This is a commercial toolament_clang_format
: This is a configurable formatter that ships with a preset for the Google Style guide. I used it before and liked it, but I don't know how it compares to uncrustify
.clang-tidy
The ament_clang_tidy
package seems to do nothing, but it can be used manually:
--cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=1
clang-tidy <cpp file paths> -p build/compile_commands.json
It is sometimes slow, but can be extremely valuable. In pose_initializer
it found nothing, but that is a tiny package. In surround_obstacle_checker
however it pointed out that in rclcpp::Duration(0.5)
, 0.5 is rounded to 0 – it accepts nanoseconds, not seconds, so this was a bug. It also pointed out an unnecessary copy, an unused argument to a function, and the use of potentially-moved-from objects.
In theory, you could compile your tests with these, just add --cmake-args -DCMAKE_CXX_FLAGS="-fsanitize=<address/undefined/thread>"
to colcon build
. However, they all report errors already for the minimal pub-sub example. I don't know if this is due to actual bugs or false positives. At least TSan can not be used without recompiling ROS2:
ThreadSanitizer generally requires all code to be compiled with -fsanitize=thread. If some code (e.g. dynamic libraries) is not compiled with the flag, it can lead to false positive race reports, false negative race reports and/or missed stack frames in reports depending on the nature of non-instrumented code.
Therefore I don't think they can be recommended without further investigation (e.g. if it's possible to disable errors coming from outside the current package).
https://github.com/preslavmihaylov/todocheck links TODO comments with GitLab issues, which would be even better than just having usernames in there. That's difficult to do while we're still transitioning from GitHub to GitLab though, and it would also be a lot of work.
There are four code linters/formatters: ament_cppcheck
, ament_cpplint
, ament_uncrustify
and ament_clang_format
. While it is in my opinion a bit awkward to have four separate tools for that, and they are mutually incompatible for some corner cases (1 and 2), they generally help. Getting linters to pass takes probably less than 10 minutes for a small package like pose_initializer
, but adds up to a lot of work for the whole code base.
Not that satisfying these linters does not mean at all that the code follows the style guide that we use, which is a slightly modified Google C++ Style guide (because that's what AutowareAuto uses, because that's what ROS uses). There are things that are addressed by none of them, e.g. that code should live in a namespace, which we should still do.
clang-tidy
is more powerful since it actually builds your code. I don't know if it's fast enough to run as part of all tests, but it seems worth at least to go through all errors it reports once and fixing them – my impression is that the ratio of effort to quality is pretty good. Maybe @fred-apex-ai can comment more on it since he has used it in the past.
My suggestion is to try to batch-migrate license comments, header filenames, and #include
style. We should probably also track all TODO
comments in an issue to add explanations and assignees. Then we can go through all packages and fix lint issues and run clang-tidy
. And of course, there is also tests, a good overall design, build quality, and documentation etc. that we should try to improve as part of code quality efforts.
Requiring header guards instead of #pragma once I didn't find a tool to generate those automatically, so it would potentially mean a moderate amount of work for very little gain, at least where #pragma once is already there.
@nnmm There is a good tool that I have used for this purpose in the past and on at least one package during porting until I decided this should be done in a batch operation later. It's https://github.com/cgmb/guardonce. THe workflow would be to first convert everything to #pragma once
to have a common ground, then convert to include guards based on the path in the repo. But we can/should do that in autoware.auto when the position in the directory structure would change again. Pseudocode
for each header:
guard2once header
once2guard header -p "format based on file name"