| Issue |
148242
|
| Summary |
Command in formatter check's failed comment is not very useful and can be confusing
|
| Labels |
infrastructure
|
| Assignees |
|
| Reporter |
DavidSpickett
|
I had a PR with many commits in it that failed the formatter on an update and it posted:
```
⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️
You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions h -- flang/include/flang/Common/format.h
View the diff from clang-format here.
diff --git a/flang/include/flang/Common/format.h b/flang/include/flang/Common/format.h
index ed12e3a5f..8b4ffe63c 100644
--- a/flang/include/flang/Common/format.h
+++ b/flang/include/flang/Common/format.h
@@ -81,11 +81,12 @@ static inline bool MulOverflow(int64_t X, int64_t Y, int64_t &Result) {
// Check how the max allowed absolute value (2^n for negative, 2^(n-1) for
// positive) divided by an argument compares to the other.
if (IsNegative)
- return UX >
- (static_cast<uint64_t>(std::numeric_limits<int64_t>::max()) + uint64_t(1)) /
+ return UX > (static_cast<uint64_t>(std::numeric_limits<int64_t>::max()) +
+ uint64_t(1)) /
UY;
else
- return UX > (static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) / UY;
+ return UX >
+ (static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) / UY;
#endif
}
```
It's nice to be notified, but if I try to actually use the commands it shows, a couple of things are wrong/confusing.
1. `git-clang-format --diff HEAD~1 HEAD --extensions h -- flang/include/flang/Common/format.h`
I think this is `HEAD~1` because the CI squashes the PR's commits and maybe also rebases them onto current main. There are reasons for that, but the problem is my PR actually had more commits so it needs to be `HEAD~5` or so.
(the previous CI built the branch as is, I think, but again, not saying either strategy is good or bad overall)
I know it says "test" this command, but even so, I got no diff out of `HEAD~1` so at first glance, it failed whatever "test" was being done here and looks like the local branch has changes that the remote does not.
2. The output of that command is not applied to the files because it includes `--diff`.
Rightly or wrongly, I'm not saying it should, but you run the command and are left with a diff that is...where exactly? I ended up copying it into a patch file and applying that (maybe I could have piped it to git apply). After looking at `git diff` and `git diff --staged` a few times.
If I edit the local file, the diff won't change because it only looks at the committed state. Which is generally a good thing but here it leaves me wondering what the intent of telling PR authors about this command is.
I know we have to expect some git knowledge from contributors but I think we should improve this comment because it's being sent to so many people who are trying their best to take on board the things we suggest to them.
That said, I don't have great ideas to reword it just yet. I'll think on it.
![]()
_______________________________________________
llvm-bugs mailing list
llvm-bugs@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
