"Failed remote builds" is misleading for clang-tidy/clang-format warnings.

google / llvm-premerge-checks

CI system for premerge-testing in LLVM project

Apache License 2.0

40 stars 37 forks source link

"Failed remote builds" is misleading for clang-tidy/clang-format warnings. #110

Open sam-mccall opened 4 years ago

sam-mccall commented 4 years ago

The messages from phab/merge_guards_bot used to be important and easy-to-scan: failures indicated a serious problem that needed to be fixed.

While clang-tidy and clang-format warnings have value, using the same channel to communicate them makes it spammy:

they're much less important, and rarely reflect issues that require major rework to the code
clang-formatting on every snapshot vs before submission is particularly unimportant
some of them are effectively false positives (e.g. Protocol.h in D73218 doesn't follow LLVM style for good reasons)

As it stands, I'd rather turn off clang-tidy/clang-format if that's possible, because these unimportant/false positive messages muddy the clear and important "tests failing" signal.

sam-mccall commented 4 years ago

(Sorry, this is clearly not a "bug" - there's only one template under "new issue" and I can't remove the label)

joker-eph commented 4 years ago

I strongly disagree with disabling the clang-format check.

In particular I believe it is a zero cost for the patch author to run git clang-format before upload. It can even be entirely automated locally.

njames93 commented 4 years ago

If code intentionally doesn't follow the style, you should consider using // clang-format off and // clang-format on around the lines that shouldn't be reformatted.

I strongly disagree with disabling the clang-format check.

In particular I believe it is a zero cost for the patch author to run git clang-format before upload. It can even be entirely automated locally.

I do think there is some issue with clang-format and that is if you are using a newer version locally than what this check is using. What can happen is each version wants to reformat the code slightly differently resulting in false positives

metaflow commented 4 years ago

Hi @sam-mccall , what do you think about the current state of checks? Clang tidy is turned off in most places AFAIR, clang-format behavior didn't changed much on the other hand.

AaronBallman commented 1 year ago

FWIW, I think clang-format failures still cause confusion in practice. https://reviews.llvm.org/D136036#3863519 is an example of a user who thinks precommit CI has failed on their patch because it was complaining about unrelated formatting changes found in the file.