Eric Blake wrote:
[adding Jim to cc, as author of useless-if-before-free]
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
virHashFree follows the convention described in HACKING that XXXFree() functions can be called with a NULL argument. --- src/conf/domain_conf.c | 6 +--- src/datatypes.c | 51 +++++++++++++++------------------------------- src/qemu/qemu_process.c | 4 +-- 3 files changed, 20 insertions(+), 41 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 351daf7..e7c3409 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUT
void virDomainObjListDeinit(virDomainObjListPtr doms) { - if (doms->objs) - virHashFree(doms->objs, virDomainObjListDeallocator); + virHashFree(doms->objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable in cfg.mk, to see if that would prevent regressions. However, it appears that this two-argument free-like function is not picked up by the heuristics in the useless-if-before-free script (it only works on one-argument functions).
ACK to your patch as-is, so I pushed it. But I can't help but wonder if we could make this easier to enforce at 'make syntax-check' time by tweaking the perl script somehow.
Right. useless-if-before-free deliberately detects only one-argument free-like functions. I'm reluctant to add such a change to that script because - there are comparatively few free-like functions with more than one argument, so there's not much anticipated gain - we don't know in general whether the free'd pointer arg is the first or second, so... - having to match against either the first or 2nd argument would greatly complicate the already complicated regular expressions If you're interested in pursuing it, I suggest switching tools altogether and using spatch. That would make for a much cleaner (albeit probably less efficient) script. It would also depend on a less-ubiquitous tool than perl.