Skip to content

Conversation

@cyyever
Copy link
Contributor

@cyyever cyyever commented Jul 1, 2021

Fix some code problems found by PVS-Studio and clang-tidy analyzers.

@cyyever cyyever force-pushed the some_fixes branch 2 times, most recently from f8e6767 to 44360c8 Compare July 1, 2021 10:01
@rwgk
Copy link
Collaborator

rwgk commented Jul 2, 2021

Hi @cyyever, LGTM and I want to merge this asap (based on @Skylion007's approval and my own), but could you please add a hint to the PR description why you applied this fix? Did some tool find it (like clang-tidy)? Sanitizer? Or just something you noticed? (All of these are good reasons IMO.)

@rwgk
Copy link
Collaborator

rwgk commented Jul 2, 2021

BTW: The CI failure is a flake that I've seen several times before and we can safely ignore.

@cyyever
Copy link
Contributor Author

cyyever commented Jul 3, 2021

@Skylion007 @rwgk I have committed some more changes. Most of the tests run successfully, but please review the code.

@rwgk
Copy link
Collaborator

rwgk commented Jul 3, 2021

Before we look at the details, could you please explain why you started working on the changes, and how you picked out the places you changed?

@rwgk
Copy link
Collaborator

rwgk commented Jul 3, 2021

FWIW: The CI failures are unrelated flakes I've seen several times before.

@cyyever
Copy link
Contributor Author

cyyever commented Jul 3, 2021

@rwgk In order to achieve better code quality, I used various coda analyzers to scan the source code and checked each warning. The used tools were mentioned above. Also because pytorch and some other projects use pybind and I need to make sure pybind being as stable as possible

@rwgk
Copy link
Collaborator

rwgk commented Jul 3, 2021

@rwgk In order to achieve better code quality, I used various coda analyzers to scan the source code and checked each warning. The used tools were mentioned above. Also because pytorch and some other projects use pybind and I need to make sure pybind being as stable as possible

That sounds good. I'm glad they didn't find more! Could you please summarize in the description what those analyzers were, ideally with version numbers? That way we can reason about the changes in the future. Also, what do you think how feasible it is to integrate them into GitHub actions, so that we stay healthy in the future. (That would be separate PRs.)

@Skylion007
Copy link
Collaborator

@rwgk Looks like the Clang-Tidy check I was going to turn on, cppcoreguidelines-init-variables, . Not sure about that last diff though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants