Re: [PATCH v3 10/13] strmap: add a strset sub-type

From: Jeff King <peff@peff.net>
Date: Nov 4, 2020, 20:31 UTC
Message-ID: <20201104203117.GC3629238@coredump.intra.peff.net>
In-Reply-To: <0f57735f5e30ad61a2e6fdb118067afbcea69660.1604343314.git.gitgitgadget@gmail.com>

On Mon, Nov 02, 2020 at 06:55:10PM +0000, Elijah Newren via GitGitGadget wrote:

Show 7 quoted lines

> +int strset_check_and_add(struct strset *set, const char *str)
> +{
> +	if (strset_contains(set, str))
> +		return 1;
> +	strset_add(set, str);
> +	return 0;
> +}

With this implementation, I wonder if it is worth having such a specialized function. The value of an atomic check-and-add operation is that it can reuse the effort to hash the string for both operations (it could also reuse any open-table probing effort, but for a chained hash like our implementation, it's cheap to add a new entry to the front of the list).

I doubt it matters all that much for the use case in shortlog. Perhaps we should just open-code it there for now, and we can revisit it if another user comes up.

Show 10 quoted lines

> --- a/strmap.h
> +++ b/strmap.h
> @@ -28,6 +28,10 @@ int cmp_strmap_entry(const void *hashmap_cmp_fn_data,
>  			.map.strdup_strings = 1,                          \
>  			.default_value = 0,                               \
>  		    }
> +#define STRSET_INIT { \
> +			.map.map = HASHMAP_INIT(cmp_strmap_entry, NULL),  \
> +			.map.strdup_strings = 1,                          \
> +		    }

As with strint, this could be:

  #define STRSET_INIT { .map = STRMAP_INIT }

-Peff