-
Notifications
You must be signed in to change notification settings - Fork 56
Enable clang-tidy on the viewer #139
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
Conversation
yungyuc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increase clang-tidy coverage by moving from the Python-only extension to viewer.
| run: make cinclude | ||
|
|
||
| - name: make buildext | ||
| - name: make viewer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build the viewer binary
| ${JOB_MAKE_ARGS} \ | ||
| CMAKE_BUILD_TYPE=${{ matrix.cmake_build_type }} \ | ||
| CJOB_MAKE_ARGS="-DPYTHON_EXECUTABLE=$(which python3)" | ||
| CMAKE_ARGS="-DPYTHON_EXECUTABLE=$(which python3)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix a typo.
| - name: make run_viewer_pytest | ||
| run: | | ||
| make pytest \ | ||
| make run_viewer_pytest \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the viewer binary to test.
| clean: | ||
| rm -f $(MODMESH_ROOT)/modmesh/_modmesh$(pyextsuffix) | ||
| make -C $(BUILD_PATH) clean | ||
| .PHONY: cmake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorder the targets.
|
|
||
| .PHONY: buildext | ||
| buildext: cmake | ||
| cmake --build $(BUILD_PATH) --target _modmesh_py VERBOSE=$(VERBOSE) $(MAKE_PARALLEL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to use cmake targets.
| if(CLANG_TIDY_EXE AND USE_CLANG_TIDY) | ||
| set_target_properties( | ||
| viewer PROPERTIES | ||
| CXX_CLANG_TIDY "${DO_CLANG_TIDY}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use clang-tidy to viewer binary.
|
|
||
| # Hack: add a .clang-tidy file in the generated .rcc directory. | ||
| # See https://gitlab.kitware.com/cmake/cmake/-/merge_requests/777 | ||
| file(WRITE "${viewer_BINARY_DIR}/.rcc/.clang-tidy" "--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a better way to do it, but that's what I can find after dinner.
| public: | ||
|
|
||
| RScene(Qt3DCore::QNode * parent = nullptr) | ||
| explicit RScene(Qt3DCore::QNode * parent = nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to clang-tidy.
| @@ -0,0 +1,2 @@ | |||
| Checks: > | |||
| -cppcoreguidelines-owning-memory No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qt tends to have way too many raw pointers so I have to turn this off.
| } | ||
|
|
||
| RApplication::~RApplication() | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python finalization is taken care of elsewhere.
No description provided.