Re: [PATCH 0/5] cleaning up read_object() family of functions

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 12, 2023, 09:21 UTC
Message-ID: <230112.86fscg2jbm.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y77/T8dktee3wOA5@coredump.intra.peff.net>

On Wed, Jan 11 2023, Jeff King wrote:

Show 73 quoted lines

> On Mon, Jan 09, 2023 at 10:09:32AM -0500, Derrick Stolee wrote:
>
>> I did think that requiring callers to create their own object_info
>> structs (which takes at least four lines) would be too much, but
>> the number of new callers is so low that I think this is a fine place
>> to stop.
>
> Yeah, that was my feeling. I do wonder if there's a way to make it
> easier for callers of oid_object_info_extended(), but I couldn't come up
> with anything that's nice enough to merit the complexity.
>
> For example, here's an attempt to let the caller use designated
> initializers to set up the query struct:
>
> diff --git a/object-file.c b/object-file.c
> index 80b08fc389..60ca75d755 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1700,13 +1700,12 @@ void *repo_read_object_file(struct repository *r,
>  			    enum object_type *type,
>  			    unsigned long *size)
>  {
> -	struct object_info oi = OBJECT_INFO_INIT;
>  	unsigned flags = OBJECT_INFO_DIE_IF_CORRUPT | OBJECT_INFO_LOOKUP_REPLACE;
>  	void *data;
> +	struct object_info oi = OBJECT_INFO(.typep = type,
> +					    .sizep = size,
> +					    .contentp = &data);
>  
> -	oi.typep = type;
> -	oi.sizep = size;
> -	oi.contentp = &data;
>  	if (oid_object_info_extended(r, oid, &oi, flags))
>  	    return NULL;
>  
> diff --git a/object-store.h b/object-store.h
> index 1a713d89d7..e894cee61b 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -418,7 +418,8 @@ struct object_info {
>   * Initializer for a "struct object_info" that wants no items. You may
>   * also memset() the memory to all-zeroes.
>   */
> -#define OBJECT_INFO_INIT { 0 }
> +#define OBJECT_INFO(...) { 0, __VA_ARGS__ }
> +#define OBJECT_INFO_INIT OBJECT_INFO()
>  
>  /* Invoke lookup_replace_object() on the given hash */
>  #define OBJECT_INFO_LOOKUP_REPLACE 1
>
> But:
>
>   - it actually triggers a gcc warning, since OBJECT_INFO(.typep = foo)
>     sets typep twice (once for the default "0", and once by name). In
>     this case the "0" is superfluous, since that's the default, and we
>     could just do:
>
>       #define OBJECT_INFO(...) { __VA_ARGS__ }
>       #define OBJECT_INFO_INIT OBJECT_INFO(0)
>
>     but I was hoping to find a general technique for object
>     initializers.
>
>   - it's not really that much shorter than the existing code. The real
>     benefit of "data = read_object(oid, type, size)" is the implicit
>     number and names of the parameters. And the way to get that is to
>     provide an extra function.
>
> So I think we are better off with the code that is longer but totally
> obvious, unless we really want to add a function wrapper for common
> queries as syntactic sugar.
>
> -Peff

I agree that it's probably not worth it here, but I think you're just
tying yourself in knots in trying to define these macros in terms of
each other. This sort of thing will work if you just do:
	
	diff --git a/object-store.h b/object-store.h
	index e894cee61ba..bfcd2482dc5 100644
	--- a/object-store.h
	+++ b/object-store.h
	@@ -418,8 +418,8 @@ struct object_info {
	  * Initializer for a "struct object_info" that wants no items. You may
	  * also memset() the memory to all-zeroes.
	  */
	-#define OBJECT_INFO(...) { 0, __VA_ARGS__ }
	-#define OBJECT_INFO_INIT OBJECT_INFO()
	+#define OBJECT_INFO_INIT { 0 }
	+#define OBJECT_INFO(...) { __VA_ARGS__ }
	 
	 /* Invoke lookup_replace_object() on the given hash */
	 #define OBJECT_INFO_LOOKUP_REPLACE 1

Which is just a twist on René's suggestion from [1], i.e.:

	#define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }

In that case we always need to rely on the "args" being init'd, and the GCC warning you note is a feature, its initialization is "private", and you should never override it.

But likewise you don't need the "0" there, if the user provides an empty list that's their own fault, they should use OBJECT_INFO_INIT instead.

If they do provide arguments it's an implementation detail how any "default" arguments get init'd, if they're not clobbering any "private" arguments we're OK.

So using an explicit "0" is the same as providing nothing in the "*_ARGS()" case, in both cases we're just offloading that zero-init to the language.

The only way I think you can dig yourself into a proper hole here is if you're trying to support 0 or N args, as P99 shows that's possible, but quite complex (and not worth it, IMO).

1. https://lore.kernel.org/git/749f6adc-928a-0978-e3a1-2ede9f07def0@web.de/