Re: [PATCH 2/7] move setting of object->type to alloc_* functions

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 11/07/14 09:46, Jeff King wrote:
> The "struct object" type implements basic object
> polymorphism.  Individual instances are allocated as
> concrete types (or as a union type that can store any
> object), and a "struct object *" can be cast into its real
> type after examining its "type" enum.  This means it is
> dangerous to have a type field that does not match the
> allocation (e.g., setting the type field of a "struct blob"
> to "OBJ_COMMIT" would mean that a reader might read past the
> allocated memory).
> 
> In most of the current code this is not a problem; the first
> thing we do after allocating an object is usually to set its
> type field by passing it to create_object. However, the
> virtual commits we create in merge-recursive.c do not ever
> get their type set. This does not seem to have caused
> problems in practice, though (presumably because we always
> pass around a "struct commit" pointer and never even look at
> the type).
> 
> We can fix this oversight and also make it harder for future
> code to get it wrong by setting the type directly in the
> object allocation functions.
> 
> This will also make it easier to fix problems with commit
> index allocation, as we know that any object allocated by
> alloc_commit_node will meet the invariant that an object
> with an OBJ_COMMIT type field will have a unique index
> number.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  alloc.c         | 18 ++++++++++--------
>  blob.c          |  2 +-
>  builtin/blame.c |  1 -
>  commit.c        |  2 +-
>  object.c        |  5 ++---
>  object.h        |  2 +-
>  tag.c           |  2 +-
>  tree.c          |  2 +-
>  8 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/alloc.c b/alloc.c
> index d7c3605..fd5fcb7 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -18,11 +18,11 @@
>  
>  #define BLOCKING 1024
>  
> -#define DEFINE_ALLOCATOR(name, type)				\
> +#define DEFINE_ALLOCATOR(name, flag, type)			\
>  static struct alloc_state name##_state;				\
>  void *alloc_##name##_node(void)					\
>  {								\
> -	return alloc_node(&name##_state, sizeof(type));		\
> +	return alloc_node(&name##_state, flag, sizeof(type));	\
>  }

I don't particularly like 'flag' here. (not a massive dislike, mind you:)

Perhaps: flag->object_type, type->node_type?
Or, if that's too verbose, maybe just: flag->type, type->node?

ATB,
Ramsay Jones