Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 28, 2021, 07:55 UTC
Message-ID: <87r1d9xh71.fsf@evledraar.gmail.com>
In-Reply-To: <67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com>

On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:

I commented just now on how this API is duplicated between here & another in-flight series in https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just comments on this patch here:

Show 12 quoted lines

> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index b8d880e3626..9f75a75d1c0 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)
>  	return t->env.v;
>  }
>  
> +const char *tmp_objdir_path(struct tmp_objdir *t)
> +{
> +	return t->path.buf;
> +}

Not your fault & this pre-dates your patch, but FWIW I prefer our APIs that don't have these "hidden struct" shenanigans (like say "struct string_list") so a caller could just access this, we can just declare it "const" appropriately.

We're also all in-tree friends here, so having various accessors for no reason other than to access struct members seems a bit too much.

All of which is to say if you + Neeraj come up with some common API I for one wouldn't mind moving the struct deceleration to tmp-objdir.h, but it looks like in his version it can stay "private" more easily.

In this particular case I hadn't understood on a previous reading of tmp-objdir.c why this "path" isn't a statically allocated "char path[PATH_MAX]", or a least we that hardcoded "1024" should be a PATH_MAX, as it is in some other cases.

But I digress :)