On Wed, Oct 30, 2019 at 9:27 AM shuah shuah@kernel.org wrote:
On 10/30/19 4:42 AM, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
Just make it a black list of the 5 most common for_each macros.
How does black listing work in the context of checkpatch.pl?
In the meantime, I'm sending out v7 which replaces "for_each" with "for__each" (adding the extra underscore), so that checkpatch is happy.
This change is required just to quiet checkpatch and I am not happy about asking for this change. At the same time, I am concerned about git hooks failing on this patch.
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
It would be great if we can consistently rely on a tool that is used as a criteria for patches to accept patches.
Agreed. I can see the point of not wanting to write an exception into checkpatch for every exception of it's general rules; however, it would be nice if there was a way to maybe have a special comment or something that could turn off a checkpatch error. That way, a checkpatch error/warning always means some action should be taken, and if a rule is being ignored, there is always documentation as to why.
Otherwise, I don't feel strongly about this.
Cheers