chore: Fix clang-tidy warnings#197
Conversation
Summary of ChangesHello @junrushao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on resolving and managing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses several clang-tidy warnings. Most changes involve adding NOLINT comments to suppress warnings that appear to be false positives, particularly for iterator parameters and C-style macros. A good change is the switch from NOLINTNEXTLINE to NOLINTBEGIN/NOLINTEND blocks for better scoping. There are also correct code fixes, such as passing an ObjectRef-derived type by const reference instead of by value to improve performance, and removing an unnecessary reinterpret_cast to void*. The changes are sound and improve the codebase. I have one minor suggestion regarding a leftover comment.
| "Class must be derived from ObjectRef or Object"); | ||
| if constexpr (std::is_base_of_v<ObjectRef, Class>) { | ||
| auto fwrap = [func](const Class target, Args... params) -> R { | ||
| auto fwrap = [func](const Class& target, Args... params) -> R { |
There was a problem hiding this comment.
@tqchen This line may be a little concerning. Could you take a look just in case?
| .def("testing.get_mlir_add_one_c_symbol", | ||
| []() { | ||
| return reinterpret_cast<int64_t>(reinterpret_cast<void*>(_mlir_add_one_c_symbol)); | ||
| []() { // |
There was a problem hiding this comment.
in some systems it is important to first cast function pointer to void* then cast to int64_t
junrushao
force-pushed
the
2025-10-27/clang-tidy-next
branch
from
d03f261 to
215f4ad
Compare
Depends on #197. This PR adds clang-tidy to per-commit tests on branch `main` to detect potential breaks.
2 participants
Following #78, this PR addresses new warnings from clang-tidy.
After this PR being merged, PR #95 will run clang-tidy tests per-commit to
mainbranch to make sure it always passes.