Re: [PATCH v2 0/8] object_id part 4

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jun 19, 2016, 17:25 UTC
Message-ID: <20160619172538.GA16194@vauxhall.crustytoothpaste.net>
In-Reply-To: <20160619092448.GA12221@sigill.intra.peff.net>

On Sun, Jun 19, 2016 at 05:24:48AM -0400, Jeff King wrote:

Show 8 quoted lines

> On Sun, Jun 19, 2016 at 10:50:14AM +0200, Johannes Sixt wrote:
> > To avoid future mistakes, can you write down how "transform plain structs
> > before pointers to structs" looks like? Is it a particular order of
> > Coccinelle rules? Which part of the interdiff between the previous round and
> > this round makes the difference?
> 
> Yeah, I'd also like a better understanding of what went wrong in the
> original (just to be able to better evaluate the tool).

I'm not completely clear on why Coccinelle made the transform that it did. If you look at the commit messages, the new patch is this:

  @@
  struct diff_filespec o;
  @@
  - o.sha1
  + o.oid.hash

  @@
  struct diff_filespec *p;
  @@
  - p->sha1
  + p->oid.hash

whereas the two pieces were reversed before. This fixes the problem because it's never possible for the second transform to match after the first has transformed a given piece of code. The functional interdiff shows that only the issues the two of you found are changed.

> Also, for the record, the issue was noticed by Johannes originally, not
> me, as indicated above (I just found a similar case after seeing his
> report).

I apologize for the misattribution.

Show 18 quoted lines

> > On a tangent, I wondered recently, why we need oidcpy() and oidclr(). After
> > all, in place of, e.g.,
> > 
> > 	oidcpy(&pair->two->oid, &p->oid);
> > 	oidclr(&one->oid);
> > 
> > we can write
> > 
> > 	pair->two->oid = p->oid;
> > 	one->oid = null_oid;
> > 
> > Is there a particular reason *not* to make this transition? I find the
> > latter less cluttered with equal clarity.
> 
> I think traditionally we've avoided struct assignment because some
> ancient compilers didn't do it. But it's in C89, and I suspect it's
> crept into the code base anyway over the years without anyone
> complaining.

I asked about this earlier and Junio felt that struct assignment was less desirable.

Also, if we do struct assignment, if we add a new hash, we may copy more than necessary (e.g. 64 bytes when we're only using SHA-1); however, this may be offset by the ability of the compiler to compute the offset at compile time.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204