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

Update clang-tidy github action #6116

Merged
merged 2 commits into from
Sep 15, 2024

Conversation

DIlkhush00
Copy link
Contributor

This PR fixes #6115

@DIlkhush00
Copy link
Contributor Author

@mvieth, any idea on how I can fix this?

@larshg
Copy link
Contributor

larshg commented Aug 24, 2024

You can see the error log on the azure site, if click on details and further. Ie https://github.com/PointCloudLibrary/pcl/actions/runs/10539455724/job/29203686331?pr=6116#step:4:4778

It seems the boolean expression can be simplified. Unfortunately I dont have time to figure out how exactly.

@larshg
Copy link
Contributor

larshg commented Aug 26, 2024

You can see the error log on the azure site, if click on details and further. Ie https://github.com/PointCloudLibrary/pcl/actions/runs/10539455724/job/29203686331?pr=6116#step:4:4778

It seems the boolean expression can be simplified. Unfortunately I dont have time to figure out how exactly.

Just had a look on my PC as well - there are quite a lot of changes required...

@DIlkhush00
Copy link
Contributor Author

You can see the error log on the azure site, if click on details and further. Ie https://github.com/PointCloudLibrary/pcl/actions/runs/10539455724/job/29203686331?pr=6116#step:4:4778
It seems the boolean expression can be simplified. Unfortunately I dont have time to figure out how exactly.

Just had a look on my PC as well - there are quite a lot of changes required...

Yes, that's what I meant. To be precise, there are a total of 419 errors. I skimmed through the logs and found 5 types of errors overall.

  1. error: use emplace instead of push [modernize-use-emplace,-warnings-as-errors] - only one error

  2. error: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty,-warnings-as-errors] - only one error

  3. error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors] - 40 errors

  4. error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors] - most of the errors (a total of 328) are of this type but will only require replacing keywords.

  5. error: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr,-warnings-as-errors] - 49 errors. This one might take a little more time to resolve since it requires figuring out how to simplify the expressions.

I'll try to make the necessary changes whenever possible, but it might take a few days to resolve all of them.

P.S. If I'm missing anything from the above list, please let me know.

@larshg
Copy link
Contributor

larshg commented Aug 26, 2024

When I search for: Error: it shows 100 entries from the log.
Please make new Prs - maybe for each type of errors you listed. Or atleast one new PR, with ie. 5 commits with each type in each commit.
That would make it easier to review 😄

@mvieth
Copy link
Member

mvieth commented Aug 27, 2024

When I search for: Error: it shows 100 entries from the log.

I believe the search only reports 100 matches at most, even if there are more

Please make new Prs - maybe for each type of errors you listed. Or atleast one new PR, with ie. 5 commits with each type in each commit. That would make it easier to review 😄

I second this: please make sure that the PR(s) don't become too large.
It seems that some errors are reported multiple times, so hopefully there are many duplicates among the 419 reported errors. 🙂

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@larshg larshg merged commit aabe846 into PointCloudLibrary:master Sep 15, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update our clang-tidy github action
3 participants