Eric Blake <ebl...@redhat.com> writes: > On 01/20/2016 11:49 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> Similar to the previous patch, it's nice to have all functions >>> in the tree that involve a visitor and a name for conversion to >>> or from QAPI to consistently stick the 'name' parameter next >>> to the Visitor parameter. >>> >>> Done by manually changing include/qom/object.h and qom/object.c, >>> then running this Coccinelle script and touching up the fallout >>> (Coccinelle insisted on adding some trailing whitespace). >>> >>> @ rule1 @ >>> identifier fn; >>> type Object, Visitor, Error; >>> identifier obj, v, opaque, name, errp; >>> @@ >>> void fn >>> - (Object *obj, Visitor *v, void *opaque, const char *name, >>> + (Object *obj, Visitor *v, const char *name, void *opaque, >>> Error **errp) { ... } >> >> I think we want to match void functions with exactly these parameter >> types. The parameter names don't matter. > > The parameter names shouldn't matter; the 'identifier obj' should have > been enough to make 'obj' a metavariable matching any actual parameter name. > >> >> However, the actual match is looser! For instance, it also matches >> >> void foo(int *pi, unsigned *pu, void *vp, const char *cp, double **dpp) >> { >> } > > Uggh. My intent was to match exactly 'Object *' and 'Visitor *' as the > first two types, where 'int *' and 'unsigned *' are NOT matches. But I > don't know Coccinelle well enough to make that blatantly obvious (is my > declaration of 'type Object' not correct?).
'type Object' makes 'Object' a metavariable matching any C type. >> This could mess up unrelated function. I could double-check it doesn't, >> but I'd rather have a narrower match instead. Can't give one offhand, >> though. Ideas? > > Is 'typedef' better than 'type' for constraining the type of the first > two arguments? Matches any C typedef name. Less wrong, but still wrong :) > Or does Coccinelle do literal matches on anything you > don't pre-declare, as in: Yes, but... > @ rule1 @ > identifier fn; > identifier obj, v, opaque, name, errp; > @@ > void fn > - (Object *obj, Visitor *v, void *opaque, const char *name, > + (Object *obj, Visitor *v, const char *name, void *opaque, > Error **errp) { ... } ... when I try that, spatch throws a parse error. > Fortunately, a manual inspection of the results (which I had to do > anyways due to spacing issues) didn't spot any incorrect swaps. > > At this point, I don't know that re-writing Coccinelle will be worth the > hassle (nothing else needs to be rewritten). I'd normally take up the challenge to wrestle Coccinelle, but I think our time is better spent elsewhere right now. Let's amend the commit message instead: describe the semantic patch's shortcomings, and your manual checking: This semantic patch is actually far too lose: it matches *any* type, not just Object, Visitor, Error. However, I can't figure out how to make Coccinelle match Object, Visitor, Error exactly. Simply deleting the 'type Object, Visitor, Error;' line results in a parse error. I reviewed the resulting patch carefully to verify there are no unwanted matches. >>> @@ >>> identifier rule1.fn; >>> expression obj, v, opaque, name, errp; >>> @@ >>> fn(obj, v, >>> - opaque, name, >>> + name, opaque, >>> errp) >> >> The rule1.fn restricts the match to functions changed by the previous >> rule. Good. >> >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> >>