On Mon, 24 Feb 2025 at 20:09, Jeff Johnson <jeff.johnson@xxxxxxxxxxxxxxxx> wrote: > > On 1/15/2025 7:33 AM, Brendan Jackman wrote: > > Checkpatch sometimes has false positives. This makes it less useful for > > automatic usage: tools like b4 [0] can run checkpatch on all of your > > patches and give you a quick overview. When iterating on a branch, it's > > tiresome to manually re-check that any errors are known false positives. > > > > This patch adds a feature to record in the patch "graveyard" (after the > > "---" that a patch might produce a certain checkpatch error, and that > > this is an expected false positive. > > > > Note, for Git users this will require some configuration changes to > > adopt (see documentation patch), and not all tools that wrap Checkpatch > > will automatically be able to take advantage. Changes are required for > > `b4 prep --check` to work [0], I'll send that separately if this patch > > is accepted. > > > > [0] https://github.com/bjackman/b4/tree/checkpatch-ignore > > While this addresses one issue with checkpatch, it doesn't solve what I > consider to be a bigger issue, namely to have a way for checkpatch to ignore > false positives (or deliberate use of non-compliant code) when run with the -f > flag. > > I don't know how many times I've seen new kernel contributors try to "fix" > checkpatch issues as their first commit, and contribute broken code in the > process. This is especially true when trying to "fix" macros. > > So IMO what would be more useful is to have some way to document these > overrides in the tree so that they could be honored both when processing > patches as well as when processing files. > > Note that thanks to Kalle's work, the wireless/ath drivers have their own way > of doing this, but of course that only works if you run the scripts. > checkpatch run normally would still flag the issues. > > more surgical: > <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath12k/ath12k-check> > > less surgical: > <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath11k/ath11k-check> > <https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check> Yeah I think the best solution for that would be something like a .checkpatch-ignore file in the spirit of .gitignore. Maybe other tools have solutions for that that checkpatch could copy. The only one I know of is pylint which solves it with code comments. That makes a real visual mess of the code in my experience. And based on Konstantin's comments on v1 of this patch, comments _definitely_ wouldn't fly with the kernel community! So I think it would have to be an out-of-band config, and if that comes at the expense of granularity then so be it. (I would support the inline-config approach for a very high-precision linter, like Rust and Go have, but Checkpatch Dot P.L. is never gonna be one of those). Anyway, solving the -f case shouldn't be required for merging this feature IMO.
