On 27/11/17 11:57 AM, Joe Perches wrote:
It may or not be correct.
It's absolutely not correct in that it either requires that a subsequent KERN_CONT/pr_cont or a '\n' at the end and it has neither.
Without inter-function call code flow analysis, it's not possible to be correct.
But how many cases actually have the pr_cont/KERN_cont called in different functions? This appears to be exceedingly rare to me.
If you can get the false positive & false negative rate higher, I'll listen.
The only two classes of false positives that you've pointed out or that I'm aware of:
1) The case where call did not either end in a '\n' or have a KERN_CONT/pr_cont in a subsequent call. I've been arguing (to deaf ears) that a warning is appropriate here and this is not a false positive because it absolutely is incorrect one way or the other. Coccinnelle will also suffer from this issue because it can no better decide whether the developer intended for the next call to be a continuation or for a '\n' to end the line.
2) Cases where the pr_cont/KERN_CONT is not in sufficient context for the script to detect. These are impossible to fix (and it's likely also impossible for Coccinelle to be 100% accurate here). However, I'd expect these to be *very* rare and I'm only actually aware of one case where this has actually happened (lib/locking-selftest.c:1189) and (mostly by luck) my v2 patch does not flag this where Coccinelle did. Not to mention that continuation usage is discouraged in new code so this should be even rarer on the majority of what checkpatch is used for.
(also 3. would be the %pV case, but I've removed those in what could be a v3 of the patch -- I'd also be happy to address other false positives classes if I could find them)
False negatives are much harder to quantify or improve. But given that I detect nearly 6000 errors in the existing kernel it can't be *that* high. Also, these false negatives do nothing to negate the benefit of having this functionality seeing the vast majority of developers are doing simple things with pr_* and dev_*.
Coccinelle may very well be able to do better at false negatives. But in this case, it would still be great to have both because checkpatch will flag a significant subset of the errors much earlier in the development cycle and save developers a bunch of time.
So, in my opinion, I think focusing too hard on the false negatives deprives developers of what could be a useful check.
I think the Coccinelle script has a better chance to be more correct.
And yet, you have not pointed out any false positives that my patch gives which Coccinelle does/would not. It really feels to me like your biases are guiding your decision here and you aren't really looking at the results.
Another thought I've had is that the dev_ functions don't have any form of continuation. So we could potentially limit checkpatch to looking for those to avoid the issues with continuations. It's not high coverage but at least a lot of the driver patches would be checked with no chance of false positives. I think there would be value in doing that.
Logan -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
