Skip to content
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

Comprehensive linting and formatting #821

Merged
merged 9 commits into from
Dec 4, 2023
Merged

Comprehensive linting and formatting #821

merged 9 commits into from
Dec 4, 2023

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Dec 2, 2023

This PR implements "comprehensive" linting and formatting for both Python and C/C++. It runs

  • black and pylint for Python;
  • clang-format and clang-tidy for C/C++.

Notes:

  • All checking/linting/formatting is only on changed lines.
  • Annotations are posted on the changed lines themselves.
  • Lint issues are warnings/suggestions (and thus applying is optional).
  • Formatting issues are errors and need to be applied (or fixed locally).
  • Annotations/suggestions can be batch committed from the Files changed tab.

This PR itself serves as a test; I made "anti-changes" to lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp and python/util.py to exercise the formatters and linters. Thus (hopefully), this PR doesn't accurately represent the typical amount of noise the linters introduce.

Note: I do not intend to "own" this code. This is a shared resource and should be shared responsibility.

lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
@makslevental makslevental force-pushed the fix_clang_tidy branch 2 times, most recently from d7484b0 to fabd563 Compare December 2, 2023 20:57
@makslevental makslevental marked this pull request as draft December 2, 2023 20:57
Copy link
Contributor

github-actions bot commented Dec 2, 2023

Coverage Report

Created: 2023-12-04 22:11

Click here for information about interpreting this report.

FilenameFunction CoverageLine CoverageRegion CoverageBranch Coverage
home/runner/work/mlir-aie/mlir-aie/lib/Dialect/AIE/IR/AIEDialect.cpp 92.79% 83.83% 86.14% 75.44%
Totals 92.79% 83.83% 86.14% 75.44%
Generated by llvm-cov -- llvm version 14.0.0

@makslevental makslevental force-pushed the fix_clang_tidy branch 2 times, most recently from 587e4b0 to 7460399 Compare December 2, 2023 21:17
@makslevental makslevental changed the title Comprehensive lint and formatting Comprehensive linting and formatting Dec 2, 2023
@makslevental makslevental force-pushed the fix_clang_tidy branch 5 times, most recently from d4b63fb to a924aba Compare December 2, 2023 21:37
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@Xilinx Xilinx deleted a comment from github-actions bot Dec 2, 2023
@makslevental makslevental force-pushed the fix_clang_tidy branch 2 times, most recently from 8d9aa2f to 54046fa Compare December 2, 2023 21:43
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
@makslevental makslevental force-pushed the fix_clang_tidy branch 4 times, most recently from 51dc285 to b0f195e Compare December 2, 2023 22:23
test/python/aie_ops.py Outdated Show resolved Hide resolved
test/python/aie_ops.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsetoain jsetoain left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with CI scripts nor all that auto-linting shenanigans, I'd let somebody else take a deeper look.

.clang-tidy Show resolved Hide resolved
.github/workflows/buildAndTest.yml Show resolved Hide resolved
.github/workflows/buildAndTest.yml Show resolved Hide resolved
.github/workflows/buildAndTest.yml Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
lib/Conversion/AIEVecToLLVM/AIEVecToLLVM.cpp Outdated Show resolved Hide resolved
utils/git/clang_tidy_pr.py Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
python/util.py Outdated Show resolved Hide resolved
@makslevental makslevental force-pushed the fix_clang_tidy branch 4 times, most recently from 115d31a to 96141eb Compare December 4, 2023 22:03
makslevental and others added 9 commits December 4, 2023 16:04
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -53,13 +53,13 @@ def infer_mlir_type(
if isinstance(py_val, bool):
Copy link
Contributor

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)

@@ -53,13 +53,13 @@ def infer_mlir_type(
if isinstance(py_val, bool):
return T.bool()
elif isinstance(py_val, int):
if -(2 ** 31) <= py_val < 2 ** 31:
if -(2**31) <= py_val < 2**31:
Copy link
Contributor

Choose a reason for hiding this comment

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

[pylint] reported by reviewdog 🐶
R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)

@makslevental makslevental merged commit 1c354e7 into main Dec 4, 2023
7 checks passed
@makslevental makslevental deleted the fix_clang_tidy branch December 4, 2023 22:08
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