On Mon, Dec 05 2022, Taylor Blau wrote: > On Mon, Dec 05, 2022 at 10:01:11PM +0100, René Scharfe wrote: >> This rule would turn this code: >> >> struct foo *bar = xcalloc(1, sizeof(*bar)); >> int i; >> >> ... into: >> >> struct foo *bar; >> CALLOC(bar); >> int i; >> >> ... which violates the coding guideline to not mix declarations and >> statements (-Wdeclaration-after-statement). > > Yeah, I was wondering about this myself when I wrote this part of the > Coccinelle patch. > > Is there an intelligent way to tell it to put the first statement after > all declarations? I couldn't find anything after a quick scan of the > documentation nor our own patches. We can get that working, but I think it's good to establish some ground rules first: First, when you add *.pending.cocci rules they shouldn't be pseudocode, but things that are too large to apply right now. I think my recent 041df69edd3 (Merge branch 'ab/fewer-the-index-macros', 2022-11-28) is a good example. In this case if I apply the proposed patch and do: make contrib/coccinelle/xcalloc.pending.cocci.patch && patch -p1 <contrib/coccinelle/xcalloc.pending.cocci.patch && make We have a deluge of syntax errors, not just the warnings René notes. This needs to be fixed, the *.pending.cocci must actually work on our code. (We should probably modify the "static-analysis" to actually apply the result of these *.pending patches and compile with those, but until then we can manually test them). Second, we have test support for rules now, see contrib/coccinelle/tests/. You just need to create a contrib/coccinelle/tests/xcalloc.pending.c, and have the expected post-image in contrib/coccinelle/tests/xcalloc.pending.res. Please add one of those. We don't have them for existing rules, but it really helps to assert & review new rules. The various edge cases that your current *.cocci doesn't compile on etc. should go into that test file. Third, the resulting patch is currently ~2k lines. Can we really not make it non-pending with some whitelisting/gblacklisting. E.g. see this out-of-tree patch for an example of opting out certain functions: https://github.com/avar/git/commit/bedd0323e9b I think that's preferable to having some *.pending.cocci we may or may not get to. I think you can also probably write a working rule for this to incrementally target certain subsets of these, and we could just mass-convert some stuff already (if it doesn't conflict with in-flight etc.). Fourth, I must say I'm kind of negative on the proposed change. I.e. the foot-gun is definitely worth solving, but why not just create a coccinelle rule to narrowly address that? In that case we can presumably start with it non-pending, as we don't have that many of them. On the notion that we need to special-case: - CALLOC_ARRAY(ptr, 1) + CALLOC(ptr) Because an array of one is "not an array" I don't really buy it. The calloc() interface itself exposes that, so I don't see why we'd consider those separate on the API level for these wrapper macros. I.e. I think the point of the macro should be to provide the appropiate sizeof() for you, not to be opinionated that nmemb=1 should be special-cased.I think it's probably not worth the churn to make that transformation. But if we *are* doing that then surely we should provide the full set of functions. I.e. ALLOC() and ALLOC_ARRAY(), CALLOC() and CALLOC_ARRAY(), and REALLOC() and REALLOC_ARRAY()?
