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.

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

refactor(map_projection_loader): apply static analysis #7497

Conversation

a-maumau
Copy link
Contributor

@a-maumau a-maumau commented Jun 14, 2024

Description

This PR applies some suggestions from the linter to map/map_projection_loader.
Checked with clang-tidy and cppcheck.

Fixed:

  • use raw string literal instead of escaped string literal
  • removed and merged repeated branch
    • the condition was different but the branch was same (doing the same assigning)

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_projection_loader
...
27361 warnings generated.
Suppressed 27455 warnings (27361 in non-user code, 94 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
30161 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_projection_loader/test/test_load_info_from_lanelet2_map.cpp:44:11: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
  file << "  <node id=\"1\" lat=\"" << lat << "\" lon=\"" << lon << "\"/>\n";
          ^~~~~~~~~~~~~~~~~~~~~~~~~
          R"(  <node id="1" lat=")"
Suppressed 30283 warnings (30160 in non-user code, 123 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
13905 warnings generated.
Suppressed 13916 warnings (13905 in non-user code, 11 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
48492 warnings generated.
Suppressed 48586 warnings (48492 in non-user code, 94 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
36281 warnings generated.
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_projection_loader/src/map_projection_loader.cpp:36:96: warning: repeated branch in conditional chain [bugprone-branch-clone]
  } else if (msg.projector_type == tier4_map_msgs::msg::MapProjectorInfo::LOCAL_CARTESIAN_UTM) {
                                                                                               ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_projection_loader/src/map_projection_loader.cpp:42:4: note: end of the original
  } else if (msg.projector_type == tier4_map_msgs::msg::MapProjectorInfo::TRANSVERSE_MERCATOR) {
   ^
/home/masakibaba/autoware/src/universe/autoware.universe/map/map_projection_loader/src/map_projection_loader.cpp:42:96: note: clone 1 starts here
  } else if (msg.projector_type == tier4_map_msgs::msg::MapProjectorInfo::TRANSVERSE_MERCATOR) {
                                                                                               ^
Suppressed 36385 warnings (36280 in non-user code, 105 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/load_info_from_lanelet2_map.cpp ...
1/3 files checked 22% done
Checking src/map_projection_loader.cpp ...
2/3 files checked 62% done
Checking test/test_load_info_from_lanelet2_map.cpp ...
3/3 files checked 100% done

After this PR:

Check with check_linter.sh
$ ./check_linter.sh map_projection_loader
...
27361 warnings generated.
Suppressed 27455 warnings (27361 in non-user code, 94 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
30160 warnings generated.
Suppressed 30283 warnings (30160 in non-user code, 123 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
13905 warnings generated.
Suppressed 13916 warnings (13905 in non-user code, 11 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
48492 warnings generated.
Suppressed 48586 warnings (48492 in non-user code, 94 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
36280 warnings generated.
Suppressed 36385 warnings (36280 in non-user code, 105 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: no change

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 14, 2024
@SakodaShintaro SakodaShintaro added run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) tag:run-clang-tidy-differential labels Jun 17, 2024
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.

I have confirmed that logging_simulator and e2e_simulator (AWSIM) works well.
Thanks!
Looks Good To Me

@SakodaShintaro SakodaShintaro merged commit 2de41ba into autowarefoundation:main Jun 17, 2024
25 checks passed
@a-maumau a-maumau deleted the mau/refactor/map/map_projection_loader branch June 17, 2024 04:22
simon-eisenmann-driveblocks pushed a commit to simon-eisenmann-driveblocks/autoware.universe that referenced this pull request Jun 26, 2024
…ation#7497)

* 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>
Co-authored-by: SakodaShintaro <[email protected]>
Signed-off-by: Simon Eisenmann <[email protected]>
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
* 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>
Co-authored-by: SakodaShintaro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:map Map creation, storage, and loading. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants