Re: [PATCH 3/5] commit: add commit_generation function

From: Jeff King <peff@peff.net>
Date: Jul 11, 2011, 21:10 UTC
Message-ID: <20110711211013.GA30155@sigill.intra.peff.net>
In-Reply-To: <20110711175709.GA6245@toss.lan>

On Mon, Jul 11, 2011 at 07:57:09PM +0200, Clemens Buchacher wrote:

Show 10 quoted lines

> On Mon, Jul 11, 2011 at 12:18:14PM -0400, Jeff King wrote:
> >
> > +unsigned long commit_generation(const struct commit *commit)
> > +{
> [...]
> > +	/* drop const because we may call parse_commit */
> > +	return commit_generation_recurse((struct commit *)commit);
> > +}
> 
> Out of curiosity, why make it const in the first place?

Two reasons:

  1. At the API layer, it's conceptually const. We're not changing the
     commit, but the lazy load of parse_commit is an implementation
     detail.

     In C++, you would stick a "mutable" tag on the parts we lazily load
     via parse_object. Here, we have to cast.

     This isn't C++, of course, and while we do follow some
     object-oriented principles, it's not necessarily worth fighting the
     language like this for the sake of a const. And I would be fine
     with saying "all commit objects should not be marked const, because
     we may lazily parse them, and it's well known that they are not to
     be freed anyway".

     But...

  2. The callsite in pretty.c has a const commit, so we have to cast
     somewhere, and this spot seemed the most appropriate to me (or we
     could drop the consts there, which I would be OK with).

-Peff