On Mon, Jul 11 2022, Jeff King wrote: > On Tue, Jul 05, 2022 at 03:46:54PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> This series adds a coccinelle rule to find and remove code where the >> only reference to a variable in a given function is to malloc() & >> free() it, where "malloc" and "free" also match >> "strbuf_init/strbuf_release", and then later in the series anything >> that looks like a init/free pattern. > > As before, I'm perfectly happy with the actual transformations here. Thanks! >> Changes since v3[1] >> >> * Add a "coccicheck-test" target in an early and new patch, the >> structure mirrors that of coccinelle.git's own tests. As the >> diffstat shows we have a *.c and *.res file which is C code >> before/after a *.cocci rule is applied. > > I have mixed feelings on this. I'm not opposed per se, and I even like > the fact that it provides examples of what we expect a rule to do. But I > worry about a world where the cost of using coccinelle goes up because > now transformations need to have tests along with them. The idea of > cocci is to make your life a little simpler and more efficient when > fixing up the code base (as opposed to perl, etc). That's fair, and I suppose we could add some really complex tests that e.g. include the original headers. But right now these just work on stand-alone *.c files with no includes, and the runtime is thus trivial. So I thought it was OK to run it along with the main job. > But if it comes with a bunch of boilerplate tasks, that might no > longer be the case. I'm already often on the fence about using cocci > just because of the hours I've sunk into debugging and puzzling things > out. > > I dunno. Maybe the bar is higher for stuff like this that we expect to > continue to find problems as time goes on (because it is not "here is a > transition", but "this is a common and easy mistake to make"). > > So again, I'm not really opposed to this patch in particular, but I want > to express caution at the direction we might be going, and at applying > new rules over-zealously. Yeah, we definitely need to be more careful with "ongoing" rules like these, but hopefully between the tests & lack of false positives this one looks sane... >> * We now catch init/reset patterns as well as init/release fully >> (i.e. for the "struct strbuf" early on) > > Yeah, that makes sense. > > -Peff
