On Mon, 27 Nov 2017, Logan Gunthorpe wrote: > > > 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. Perhaps if there is a possible flow from one print to another within a single function and in both cases the format string is at least say 25 characters (completely random value), then it is pretty likely that a newline is intended. Alternatively, if the first format string doesn't end in a space and the second one doesn't begin with a space, then a newline is also likely intended. julia -- 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
