Re: [PATCH] cache_tree_find(): remove redundant check in while condition

From: David Kastrup <dak@gnu.org>
Date: Mar 3, 2014, 16:32 UTC
Message-ID: <87a9d7kztm.fsf@fencepost.gnu.org>
In-Reply-To: <1393862885-23271-1-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 20 quoted lines

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> A trivial little cleanup.
>
>  cache-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..7f63c23 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -565,7 +565,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
>  			return NULL;
>  		it = sub->cache_tree;
>  		if (slash)
> -			while (*slash && *slash == '/')
> +			while (*slash == '/')
>  				slash++;
>  		if (!slash || !*slash)
>  			return it; /* prefix ended with slashes */

That seems dragging around a NULL slash a lot. How about not checking for it multiple times?

		if (!slash)
                	return it;
		while (*slash == '/')
			slash++;
		if (!*slash)
			return it; /* prefix ended with slashes */

As a bonus, the comment appears to be actually correct. Attempting to verify its correctness by seeing whether a non-NULL slash is guaranteed to really end with slashes, we find the following code above for defining slash:

                slash = strchr(path, '/');
                if (!slash)
                        slash = path + strlen(path);

So it is literally impossible for slash to ever be NULL and all the checking is nonsensical. In addition, "prefix ended with slashes" does not seem overly convincing when this code path is reached whether or not there is a slash at all (I am not sure about it: it depends on the preceding find_subtree to some degree).

So perhaps all of that should just be

		while (*slash == '/')
			slash++;
		if (!*slash)
			return it;

-- 
David Kastrup