Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function

From: Jeff King <peff@peff.net>
Date: Jun 18, 2014, 20:08 UTC
Message-ID: <20140618200854.GA23098@sigill.intra.peff.net>
In-Reply-To: <53A1EE0E.6040000@ramsay1.demon.co.uk>

On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:

Show 19 quoted lines

> In order to encapsulate the setting of the unique commit index, commit
> 969eba63 ("commit: push commit_index update into alloc_commit_node",
> 10-06-2014) introduced a (logically private) intermediary allocator
> function. However, this function (alloc_raw_commit_node()) was declared
> as a public function, which undermines its entire purpose.
> 
> Remove the alloc_raw_commit_node() function and inline its code into
> the (public) alloc_commit_node() function.
> 
> Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
> Should it be static?").
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
> 
> Hi Jeff,
> 
> I noticed this while it was still in 'pu', but got distracted and
> didn't send this in time ... sorry about that! :(

Yeah, I noticed it while writing the patch but decided it wasn't worth the trouble to deal with (since after all, it's not advertised to any callers, the very thing that sparse is complaining about. :) ).

I don't mind fixing it, though I really don't like repeating the contents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but it just feels wrong.

Show 8 quoted lines

> My first attempt at fixing this involved changing the DEFINE_ALLOCATOR
> macro to include a 'scope' parameter so that I could declare the
> raw_commit allocator 'static'. Unfortunately, I could not pass the
> extern keyword as the scope parameter to all the other allocators,
> because that made sparse even more upset - you can't use extern on
> a function _definition_. That meant passing an empty argument (or a
> comment token) to the scope parameter. This worked for gcc 4.8.2 and
> clang 3.4, but I was a little concerned about portability.

Yeah, passing an empty argument was my first thought, but I don't know offhand if there are portability concerns.

You could also have DEFINE_ALLOCATOR just fill in the body, and do:

  struct blob *alloc_blob_node(void)
  {
	DEFINE_ALLOCATOR(struct blob);
  }

or similar.

-Peff