Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(map_tf_generator): apply static analysis #7350

Merged
merged 2 commits into from
Jun 10, 2024
Merged

refactor(map_tf_generator): apply static analysis #7350

merged 2 commits into from
Jun 10, 2024

Conversation

a-maumau
Copy link
Contributor

@a-maumau a-maumau commented Jun 7, 2024 •
edited
Loading

Description

This PR applies some suggestions from the linter to map/map_tf_generator.

Fixed:

  • camelCase to snake_case
  • variable name changed from VAR_NAME to var_name
  • use explicit cast
  • use initialization list for some variable

Not fixed:

  • using [std, boost]::variant for safe access
    • for speed concern

Tests performed

Checked with clang-tidy and cppcheck (v2.14.1)

check_linter.sh 
#!/bin/bash
set -eux

TARGET_DIR=$1

current_dir=$(basename $(pwd))
if [[ ! $current_dir =~ ^(autoware|pilot-auto) ]]; then
    echo "This script must be run in a directory with a prefix of autoware or pilot-auto."
    exit 1
fi

set +eux
export CPLUS_INCLUDE_PATH=/usr/include/c++/11:/usr/include/x86_64-linux-gnu/c++/11:$CPLUS_INCLUDE_PATH
set -eux

fdfind -e cpp -e hpp --full-path ${TARGET_DIR} | xargs -P $(nproc) -I{} cpplint {}
fdfind -e cpp -e hpp --full-path ${TARGET_DIR} | xargs -P $(nproc) -I{} clang-tidy -p build/ {}

Before fixing the code:

output from above commands 
./check_linter.sh map_tf_generator
...
3453 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/include/map_tf_generator/uniform_random.hpp:21:21: warning: function 'UniformRandom' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
std::vector<size_t> UniformRandom(const size_t max_exclusive, const size_t n)
                    ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/include/map_tf_generator/uniform_random.hpp:21:21: note: make as 'inline'
std::vector<size_t> UniformRandom(const size_t max_exclusive, const size_t n)
                    ^
inline 
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/include/map_tf_generator/uniform_random.hpp:21:21: warning: invalid case style for function 'UniformRandom' [readability-identifier-naming]
std::vector<size_t> UniformRandom(const size_t max_exclusive, const size_t n)
                    ^~~~~~~~~~~~~
                    uniform_random
Suppressed 3451 warnings (3451 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8967 warnings generated.
Suppressed 8998 warnings (8967 in non-user code, 31 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
43025 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:30:18: warning: invalid case style for constexpr variable 'N_SAMPLES' [readability-identifier-naming]
constexpr size_t N_SAMPLES = 20;
                 ^~~~~~~~~
                 n_samples
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:56:8: warning: invalid case style for function 'onPointCloud' [readability-identifier-naming]
  void onPointCloud(const sensor_msgs::msg::PointCloud2::ConstSharedPtr clouds_ros)
       ^~~~~~~~~~~~
       on_point_cloud
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:68:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[0] += clouds.points[i].x;
                                        ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:69:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[1] += clouds.points[i].y;
                                        ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:70:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[2] += clouds.points[i].z;
                                        ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:72:37: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
    coordinate[0] = coordinate[0] / indices.size();
                                    ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:73:37: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
    coordinate[1] = coordinate[1] / indices.size();
                                    ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:74:37: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
    coordinate[2] = coordinate[2] / indices.size();
                                    ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:76:42: warning: invalid case style for variable 'static_transformStamped' [readability-identifier-naming]
    geometry_msgs::msg::TransformStamped static_transformStamped;
                                         ^~~~~~~~~~~~~~~~~~~~~~~
                                         static_transform_stamped
Suppressed 43029 warnings (43016 in non-user code, 13 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
54629 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/vector_map_tf_generator_node.cpp:52:8: warning: invalid case style for function 'onVectorMap' [readability-identifier-naming]
  void onVectorMap(const autoware_map_msgs::msg::LaneletMapBin::ConstSharedPtr msg)
       ^~~~~~~~~~~
       on_vector_map
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/vector_map_tf_generator_node.cpp:69:64: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
      std::accumulate(points_x.begin(), points_x.end(), 0.0) / points_x.size();
                                                               ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/vector_map_tf_generator_node.cpp:71:64: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
      std::accumulate(points_y.begin(), points_y.end(), 0.0) / points_y.size();
                                                               ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/vector_map_tf_generator_node.cpp:73:64: warning: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
      std::accumulate(points_z.begin(), points_z.end(), 0.0) / points_z.size();
                                                               ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/vector_map_tf_generator_node.cpp:75:42: warning: invalid case style for variable 'static_transformStamped' [readability-identifier-naming]
    geometry_msgs::msg::TransformStamped static_transformStamped;
                                         ^~~~~~~~~~~~~~~~~~~~~~~
                                         static_transform_stamped
Suppressed 54700 warnings (54624 in non-user code, 76 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Check with cppcheck:

output 
$ cppcheck --enable=warning --enable=style --enable=performance --enable=portability --enable=unusedFunction --inconclusive --check-level=exhaustive .

Checking src/pcd_map_tf_generator_node.cpp ...
src/pcd_map_tf_generator_node.cpp:39:5: performance: Variable 'map_frame_' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList]
    map_frame_ = declare_parameter<std::string>("map_frame");
    ^
src/pcd_map_tf_generator_node.cpp:40:5: performance: Variable 'viewer_frame_' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList]
    viewer_frame_ = declare_parameter<std::string>("viewer_frame");
    ^
1/3 files checked 41% done
Checking src/vector_map_tf_generator_node.cpp ...
src/vector_map_tf_generator_node.cpp:34:5: performance: Variable 'map_frame_' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList]
    map_frame_ = declare_parameter<std::string>("map_frame");
    ^
src/vector_map_tf_generator_node.cpp:35:5: performance: Variable 'viewer_frame_' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList]
    viewer_frame_ = declare_parameter<std::string>("viewer_frame");
    ^
2/3 files checked 85% done
Checking test/test_uniform_random.cpp ...
3/3 files checked 100% done
test/test_uniform_random.cpp:24:0: style: The function 'TEST' is never used. [unusedFunction]
TEST(UniformRandom, UniformRandom)

After this PR:

Check with check_linter.sh

$ ./check_linter.sh map_tf_generator
...
3451 warnings generated.
Suppressed 3451 warnings (3451 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
8965 warnings generated.
Suppressed 8996 warnings (8965 in non-user code, 31 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
43017 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:67:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[0] += clouds.points[i].x;
                                        ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:68:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[1] += clouds.points[i].y;
                                        ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_tf_generator/src/pcd_map_tf_generator_node.cpp:69:41: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      coordinate[2] += clouds.points[i].z;
                                        ^
Suppressed 43027 warnings (43014 in non-user code, 13 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
54624 warnings generated.
Suppressed 54700 warnings (54624 in non-user code, 76 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Check with cppcheck (not changed):

$ cppcheck --enable=warning --enable=style --enable=performance --enable=portability --enable=unusedFunction --inconclusive --check-level=exhaustive .

Checking src/pcd_map_tf_generator_node.cpp ...
1/3 files checked 41% done
Checking src/vector_map_tf_generator_node.cpp ...
2/3 files checked 85% done
Checking test/test_uniform_random.cpp ...
3/3 files checked 100% done
test/test_uniform_random.cpp:24:0: style: The function 'TEST' is never used. [unusedFunction]
TEST(uniform_random, uniform_random)

Effects on system behavior

Not applicable.

Interface changes

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:map Map creation, storage, and loading. (auto-assigned) label Jun 7, 2024
@SakodaShintaro SakodaShintaro added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) tag:run-clang-tidy-differential labels Jun 7, 2024
@codecov Codecov
Copy link

codecov bot commented Jun 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 8.88889% with 41 lines in your changes missing coverage. Please review.

Project coverage is 11.49%. Comparing base (3d849e9) to head (540d474).
Report is 188 commits behind head on main.

Files Patch % Lines
...map_tf_generator/src/pcd_map_tf_generator_node.cpp 0.00% 21 Missing ⚠️
..._tf_generator/src/vector_map_tf_generator_node.cpp 0.00% 20 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #7350       +/-   ##
===========================================
- Coverage   15.09%   11.49%    -3.61%     
===========================================
  Files        1967        4     -1963     
  Lines      135941       87   -135854     
  Branches    42122        5    -42117     
===========================================
- Hits        20520       10    -20510     
+ Misses      92700       74    -92626     
+ Partials    22721        3    -22718
Flag Coverage Δ
differential 11.49% <8.88%> (?)
total ?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

SakodaShintaro
SakodaShintaro approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@SakodaShintaro SakodaShintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me

@SakodaShintaro SakodaShintaro added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) and removed tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) labels Jun 7, 2024
@SakodaShintaro SakodaShintaro merged commit 41ee731 into autowarefoundation:main Jun 10, 2024
64 of 70 checks passed
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
@a-maumau @pre-commit-ci @KhalilSelyan
refactor(map_tf_generator): apply static analysis (#7350)
1c8a46e 
* refactor based on linter

Signed-off-by: a-maumau <[email protected]>

* style(pre-commit): autofix

---------

Signed-off-by: a-maumau <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SakodaShintaro SakodaShintaro SakodaShintaro approved these changes

@anhnv3991 anhnv3991 Awaiting requested review from anhnv3991

@KYabuuchi KYabuuchi Awaiting requested review from KYabuuchi

@Motsu-san Motsu-san Awaiting requested review from Motsu-san

@RyuYamamoto RyuYamamoto Awaiting requested review from RyuYamamoto

@TaikiYamada4 TaikiYamada4 Awaiting requested review from TaikiYamada4

@YamatoAndo YamatoAndo Awaiting requested review from YamatoAndo

Assignees
No one assigned
Labels
component:map Map creation, storage, and loading. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a-maumau @SakodaShintaro