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

fix(pointcloud_preprocessor): fix preprocessorErrorDirective #7786

Merged
merged 14 commits into from
Jul 4, 2024

Conversation

veqcc
Copy link
Contributor

@veqcc veqcc commented Jul 2, 2024

Description

This is a fix based on cppcheck preprocessorErrorDirective warning

sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:132:2: error: #error Unsupported bitness [preprocessorErrorDirective]
#error Unsupported bitness
 ^

The ROBIN_HOOD_PRIVATE_DEFINITION_BITNESS macro is not used, right?

Related links

Parent Issue:

  • Link

How was this PR tested?

Notes for reviewers

This preprocessorErrorDirective has prevented the cppcheck for analysing robin_hood.h file, so now it has a lot of cppcheck warnings, which is not acceptable in current CI workflow. See cppcheck-differential workflow for more detail.

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

veqcc added 2 commits July 2, 2024 19:05
Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
@knzo25
Copy link
Contributor

knzo25 commented Jul 3, 2024

For this one, I think only @yukkysaito can properly give an answer (since it is related to the pickup_based_voxel_grid_downsample_filter)

Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

We are using the external file. Accepting this change would not be an issue, but reducing the differences might improve maintainability. However, since the upstream repository is publicly archived and the likelihood of future changes is very low, I think it's fine to accept this PR.

@veqcc
Copy link
Contributor Author

veqcc commented Jul 3, 2024

@yukkysaito
Thank you for approving this PR!!

Since cppcheck-differential workflow is failing due to bugporone warings, we have to fix them before merging it.
The preprocessorErrorDirective error has been preventing cppcheck analysing this file properly so far.

May I add several commits before merging?
Or, do you prefer suppressing these warnings by inline cppcheck suppression?

See for detailed warnings: https://github.com/autowarefoundation/autoware.universe/actions/runs/9758833824/job/26934249776?pr=7786

/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:425:3: warning: Member variable 'BulkPoolAllocator::mHead' is not assigned a value in 'BulkPoolAllocator::operator='. [operatorEqVarError]
  operator=(const BulkPoolAllocator & ROBIN_HOOD_UNUSED(o) /*unused*/) noexcept
  ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:425:3: warning: Member variable 'BulkPoolAllocator::mListForFree' is not assigned a value in 'BulkPoolAllocator::operator='. [operatorEqVarError]
  operator=(const BulkPoolAllocator & ROBIN_HOOD_UNUSED(o) /*unused*/) noexcept
  ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:586:8: warning: The struct 'NodeAllocator' defines member function with name 'addOrFree' also defined in its parent class 'BulkPoolAllocator'. [duplInheritedMember]
  void addOrFree(void * ptr, size_t ROBIN_HOOD_UNUSED(numBytes) /*unused*/) noexcept
       ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:4[7](https://github.com/autowarefoundation/autoware.universe/actions/runs/9758833824/job/26934249776?pr=7786#step:11:8)2:8: note: Parent function 'BulkPoolAllocator::addOrFree'
  void addOrFree(void * ptr, const size_t numBytes) noexcept
       ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:5[8](https://github.com/autowarefoundation/autoware.universe/actions/runs/9758833824/job/26934249776?pr=7786#step:11:9)6:8: note: Derived function 'NodeAllocator::addOrFree'
  void addOrFree(void * ptr, size_t ROBIN_HOOD_UNUSED(numBytes) /*unused*/) noexcept
       ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:2033:[10](https://github.com/autowarefoundation/autoware.universe/actions/runs/9758833824/job/26934249776?pr=7786#step:11:11): style: Variable 'kv' can be declared as pointer to const [constVariablePointer]
    auto kv = mKeyVals + findIdx(key);
         ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:2045:10: style: Variable 'kv' can be declared as pointer to const [constVariablePointer]
    auto kv = mKeyVals + findIdx(key);
         ^
/home/runner/work/autoware.universe/autoware.universe/sensing/pointcloud_preprocessor/src/downsample_filter/robin_hood.h:[13](https://github.com/autowarefoundation/autoware.universe/actions/runs/9758833824/job/26934249776?pr=7786#step:11:14)76:7: performance: Prefer prefix ++/-- operators for non-primitive types. [postfixOperator]
      mKeyVals++;
      ^

@yukkysaito
Copy link
Contributor

@veqcc Thank you. Since the upstream repository is publicly archived, we will need to maintain this file ourselves in the future. Considering this, I believe it would be better to fundamentally address the cppcheck issues rather than suppress them with inline cppcheck suppression.

@yukkysaito yukkysaito self-requested a review July 3, 2024 03:02
@yukkysaito
Copy link
Contributor

If there are a few additional commits, can I change it to draft until it is review ready?

@veqcc
Copy link
Contributor Author

veqcc commented Jul 3, 2024

If there are a few additional commits, can I change it to draft until it is review ready?

Of cource yes 👍

I will work on fixing blocking warnings later.

@veqcc veqcc marked this pull request as draft July 3, 2024 04:14
veqcc added 5 commits July 3, 2024 17:57
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
@veqcc
Copy link
Contributor Author

veqcc commented Jul 3, 2024

@vividf
HI, could you check the pointclouds are the same as before/after this PR?
I have fixed all the cppcheck warnigs.
Most of changes are deletions of existing codes.

@veqcc veqcc added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jul 3, 2024
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.73%. Comparing base (8df4b4c) to head (9edc719).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7786      +/-   ##
==========================================
+ Coverage   28.67%   28.73%   +0.05%     
==========================================
  Files        1587     1602      +15     
  Lines      116351   117295     +944     
  Branches    49687    49981     +294     
==========================================
+ Hits        33365    33706     +341     
- Misses      73930    74510     +580     
- Partials     9056     9079      +23     
Flag Coverage Δ *Carryforward flag
differential 10.81% <100.00%> (?)
total 28.36% <ø> (-0.32%) ⬇️ Carriedforward from 68eee61

*This pull request uses carry forward flags. Click here to find out more.

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

@vividf
Copy link
Contributor

vividf commented Jul 3, 2024

@veqcc Sure, I will test it

@veqcc veqcc marked this pull request as ready for review July 4, 2024 09:52
veqcc added 2 commits July 4, 2024 18:59
Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Copy link
Contributor

@vividf vividf left a comment

Choose a reason for hiding this comment

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

I tested the changes, and the pointcloud after this PR is the same as before.
LGTM!

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
@veqcc veqcc self-assigned this Jul 4, 2024
Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
@veqcc veqcc merged commit a1ed6c2 into autowarefoundation:main Jul 4, 2024
30 checks passed
@veqcc veqcc deleted the fix/preprocessor_error_directive branch July 4, 2024 14:01
palas21 pushed a commit to palas21/autoware.universe that referenced this pull request Jul 12, 2024
…efoundation#7786)

* fix(pointcloud_preprocessor): fix preprocessorErrorDirective

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* fix postfixOperator

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* remove unnecessary count() and contains()

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* restore bitness macros

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove operator=

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove addOrFree function

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

---------

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
Signed-off-by: palas21 <palas21@itu.edu.tr>
tby-udel pushed a commit to tby-udel/autoware.universe that referenced this pull request Jul 14, 2024
…efoundation#7786)

* fix(pointcloud_preprocessor): fix preprocessorErrorDirective

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* fix postfixOperator

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* remove unnecessary count() and contains()

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* restore bitness macros

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove operator=

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove addOrFree function

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

---------

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
* fix(pointcloud_preprocessor): fix preprocessorErrorDirective

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* fix postfixOperator

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* remove unnecessary count() and contains()

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* restore bitness macros

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove operator=

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove addOrFree function

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

---------

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
Ariiees pushed a commit to Ariiees/autoware.universe that referenced this pull request Jul 22, 2024
…efoundation#7786)

* fix(pointcloud_preprocessor): fix preprocessorErrorDirective

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* fix postfixOperator

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* remove unnecessary count() and contains()

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* restore bitness macros

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove operator=

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* remove addOrFree function

Signed-off-by: veqcc <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

* add cppcheck suppression

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>

---------

Signed-off-by: Ryuta Kambe <ryuta.kambe@tier4.jp>
Signed-off-by: veqcc <ryuta.kambe@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (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.

4 participants