On Fri, Aug 24, 2018 at 07:04:27AM -0400, Julia Lawall wrote:
> On Fri, 24 Aug 2018, Jeff King wrote:
>
> > In Git's Coccinelle patches, we sometimes want to suppress a
> > transformation inside a particular function. For example, in finding
> > conversions of hashcmp() to oidcmp(), we should not convert the call in
> > oidcmp() itself, since that would cause infinite recursion. We write the
> > semantic patch like this:
> >
> > @@
> > identifier f != oidcmp;
> > expression E1, E2;
> > @@
> > f(...) {...
> > - hashcmp(E1->hash, E2->hash)
> > + oidcmp(E1, E2)
> > ...}
>
> The problem is with how how ... works. For transformation, A ... B
> requires that B occur on every execution path starting with A, unless that
> execution path ends up in error handling code.
> (eg, if (...) { ... return; }). Here your A is the start if the function.
> So you need a call to hashcmp on every path through the function, which
> fails when you add ifs.
Thank you! This explanation (and the one below about A and B not
appearing in the matched region) helped my understanding tremendously.
> What you want is what you ended up using, which is <... P ...> which
> allows zero or more occurrences of P.
And now this makes much more sense (I stumbled onto it through brute
force, but now I understand _why_ it works).
> However, this can all be very expensive, because you are matching paths
> through the function definition which you don't really care about. All
> you care about here is the name. So another approach is
Yeah, it is. Using the pre-1.0.7 version, the original patch runs in
~1.3 minutes on my machine. With "<... P ...>" it's almost 4 minutes.
Your python suggestion runs in about 1.5 minutes.
Curiously, 1.0.4 runs the original patch in only 24 seconds, and the
angle-bracket one takes 52 seconds. I'm not sure if something changed in
coccinelle, or if my build is simply less optimized (my 1.0.4 is from
the Debian package, and I'm building 1.0.7 from source; I had trouble
building 1.0.4 from source).
> @@
> position p : script:python() { p[0].current_element != "oldcmp" };
> expression E1,E2;
> @@
>
> - hashcmp(E1->hash, E2->hash)
> + oidcmp(E1, E2)
Aha, this is exactly the magic I was hoping for. I agree this is the
best way to express it. I just had to tweak the patch to include the
position:
- hashcmp@p(E1->hash, E2->hash)
and it worked great. Unfortunately, Debian's spatch is not built with
python support. :(
I'm not sure if we (the Git project) want to make the jump to requiring
a more specific spatch. OTOH, only a handful of developers actually run
it, and the python support does seem quite useful. And 1.0.4 is rather
old at this point.
Again, thanks very much for your response. I have a much better
understanding of what's going on now, and what our options are for
moving forward.
-Peff
