Skip to content

Conversation

@dorde-antic
Copy link
Contributor

@dorde-antic dorde-antic commented Oct 8, 2025

Motivation

Resolves https://github.com/ROCm/rocMLIR-internal/issues/1882

Technical Details

This PR introduces clang format in pre-commit hook. Temporarily, yapf part is disabled (by putting it as a comment) until refactored py files are merged. Therefore, pre-commit hook won't trigger yapf on py files changes (if there are some changes) when we want it to do only clang format.

Test Plan

Tested locally

Test Result

Submission Checklist

@umangyadav umangyadav requested a review from Copilot October 10, 2025 13:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a Git pre-commit hook for clang-formatting C/C++ files. It adds a Python utility to handle clang-format operations and integrates it into the existing pre-commit hook workflow.

  • Adds a new Python utility (clangformat.py) that wraps git-clang-format functionality
  • Updates the pre-commit hook to run clang-format on C/C++ files in addition to the existing yapf formatting for Python files
  • Adds error handling and validation for required tools (yapf and clang-format)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
mlir/utils/widgets/clangformat.py New Python utility providing clang-format integration with Git workflow
.githooks/pre-commit Updated pre-commit hook to include clang-format execution and improved tool validation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dorde-antic dorde-antic marked this pull request as ready for review October 13, 2025 08:36
@dorde-antic dorde-antic requested a review from causten as a code owner October 13, 2025 08:36
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now our CI gives us a way to turn off formatting for external LLVM files that we don't necessarily want to format. Do we have a way of implementing this kind of logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can exclude external llvm files (same as we did for yapf) @justinrosner

@dorde-antic
Copy link
Contributor Author

Todo: uncomment yapf part since it's now being used

Test: if both cpp and py files are changed and separate cases only

@dorde-antic dorde-antic requested a review from Copilot October 24, 2025 22:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +47 to +48
if ! git diff --cached --quiet -- "${cxxfiles[@]}"; then
echo "Applied clang-format changes and staged them."
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

This conditional check always prints the message when there are cached changes, but doesn't provide actionable information. Since the files are already staged by line 45, this check is redundant. Consider removing this block or making it informative by indicating which files were modified.

Suggested change
if ! git diff --cached --quiet -- "${cxxfiles[@]}"; then
echo "Applied clang-format changes and staged them."
# List files that were modified and staged by clang-format
changed_cxxfiles=$(git diff --cached --name-only -- "${cxxfiles[@]}")
if [[ -n "$changed_cxxfiles" ]]; then
echo "Applied clang-format changes and staged the following files:"
echo "$changed_cxxfiles"

Copilot uses AI. Check for mistakes.
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.

4 participants