Add clang-tidy#182
Conversation
sea-bass
left a comment
sea-bass
left a comment
There was a problem hiding this comment.
Overall looks great! Seems it caught a few things already. There are some conflicts but otherwise we should go for it and can always tweak later.
My one major question is whether this pre-commit check works with and without pixi?
eholum
left a comment
eholum
left a comment
There was a problem hiding this comment.
yay for clang-tidy! is it super common to have it in pre-commit vs a build step (I don't know the answer I just know I like pre-commit to be mostly raw format checks...).
sjahr
force-pushed
the
pr-add-clang-tidy
branch
2 times, most recently
from
f4ff4af to
b8d4d57
Compare
# Conflicts: # .github/workflows/build_and_test.yml # pixi.lock
sjahr
force-pushed
the
pr-add-clang-tidy
branch
from
b8d4d57 to
7296248
Compare
| pixi run -e ci test_all | ||
| - name: Run clang-tidy | ||
| run: | | ||
| pixi run clang-tidy |
There was a problem hiding this comment.
@eholum Do I need the -e ci flag here too?
There was a problem hiding this comment.
Actually I added this. The only thing this does is use copy install vs symlink, to check the nanobind generated stub file diffs
I would add it just because otherwise it might break caches from only having built in the ci environment.
There was a problem hiding this comment.
Requesting changes on 3 main things:
- The linter seems to be using east const and const west haphazardly. I assume there are ways to enforce just one style, and it should be const west.
- We may not want matrix variables to get shrunk down to lower case. If there are ways to make exceptions here, do consider it. Although I see some potentially problematic edge cases like
G_no. Maybe just don't check this. - There are some suspicious things happening with making class member functions static that I don't understand. And a couple other freak instances like turning a string input arg from const ref to value, and a joint angle to the mathematical constant
ejust because it was close to a few decimal points.
Other less important comments inline, but this requires a little careful checking.
|
|
||
| // Create a FrameTask (high priority) | ||
| FrameTaskOptions frame_options{ | ||
| FrameTaskOptions const frame_options{ |
There was a problem hiding this comment.
Weird that the linter chose east const here. Can we move it left?
| /// @param path_pos_vecs The joint path position vectors. | ||
| /// @return The resulting cubic Hermite spline. | ||
| std::shared_ptr<toppra::PiecewisePolyPath> | ||
| static std::shared_ptr<toppra::PiecewisePolyPath> |
| PACKAGES=("$PACKAGE_NAME") | ||
| else | ||
| # All packages with C++ source code | ||
| PACKAGES=( |
There was a problem hiding this comment.
You have a variable here you could use for the error message
3 participants
Adds clang-tidy for better code quality.
@sea-bass @eholum do you have any thoughts on the following items:Do you like the checks/ are any missing or should we remove some?Should we include it via pre-commit or a dedicated CI job?Personally I like having everything running in pre-commit but the clang-tidy job increases the execution time ofpre-commit run -aso I can see why this is annoying. To me it would be acceptable though.Are you happy with the naming conventions or should change any?There are still some things I need to address and iron out but I wanted to hear your thoughts on these questions above while wrapping the last remaining things up.Clang tidy can now be run as a pixi task in to flavors: with and without the fix flag, I also added the clang-tidy check as a CI stage to the build and test job. Thanks for your feedback!