Re: [PATCH] use strchrnul() in place of strchr() and strlen()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2014, 20:54 UTC
Message-ID: <xmqqzjl3vyii.fsf@gitster.dls.corp.google.com>
In-Reply-To: <BLU0-SMTP1988500CEC43BD4C57254AB91890@phx.gbl>

Rohit Mani <rohit.mani@outlook.com> writes:

Show 6 quoted lines

> Avoid scanning strings twice, once with strchr() and then with
> strlen(), by using strchrnul(). Update the conditional expressions
> involving the return value of strchrnul() with a check for '\0'.
>
> Signed-off-by: Rohit Mani <rohit.mani@outlook.com>
> ---

Nicely done. I am not sure if you need to say the "update the conditional...", which is a logical consequence of such a conversion and goes without saying, though.

>  cache-tree.c     |   16 +++++++---------

This part may overlap with other topics in flight, but I expect the conflict resolution would be trivial.

Show 13 quoted lines

> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..21a13cf 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -121,11 +121,11 @@ void cache_tree_invalidate_path(struct cache_tree *it, const char *path)
>  
>  	if (!it)
>  		return;
> -	slash = strchr(path, '/');
> +	slash = strchrnul(path, '/');
>  	it->entry_count = -1;
> -	if (!slash) {
> +	if (*slash == '\0') {

Let's just say

	if (!*slash)

instead; it is more idiomatic (I won't repeat this for other hunks).

>  		int pos;
> -		namelen = strlen(path);
> +		namelen = slash - path;

After this "if (!*slash)", we compute "namelen = slash-path". Perhaps we can lose this assignment and the other one by hoisting it up before "if (!*slash)"?

> @@ -564,10 +562,10 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
> +		if (*slash == '\0' || !*slash)

Huh?  "The byte pointed at by 'slash' is NUL, or it is NUL"???

Other than that, looks good to me.

Thanks.