Re: [PATCH v2] cache_tree_find(): remove redundant checks

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/04/2014 10:40 AM, David Kastrup wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The beginning of the loop ensures that slash can never be NULL.  So
>> don't keep checking whether it is NULL later in the loop.
>>
>> Furthermore, there is no need for an early
>>
>>     return it;
>>
>> from the loop if slash points at the end of the string, because that
>> is exactly what will happen when the while condition fails at the
>> start of the next iteration.
> 
> Hm.  Another suggestion.  You have
> 
> 		const char *slash = strchr(path, '/');
>  		if (!slash)
>  			slash = path + strlen(path);
> [...]
> 		sub = find_subtree(it, path, slash - path, 0);
> [...]
> 		path = slash;
> 		while (*path == '/')
> 			path++;
> 	}
> 
> At the price of introducing another variable, this could be
> 
> 		const char *slash = strchr(path, '/');
>  		size_t len = slash ? slash - path : strlen(path);
> [...]
> 		sub = find_subtree(it, path, len, 0);
> [...]
>                 if (!slash)
> 			break;
> 		for (path = slash; *path == '/';)
> 			path++;
> 	}
> 
> This introduces another variable and another condition.  The advantage
> is that "slash" indeed points at a slash or is NULL, so the variable
> names correspond better to what happens.  Alternatively, it might make
> sense to rename "slash" into "end" or "endpart" or whatever.  Since
> I can't think of a pretty name, I lean towards preferring the latter
> version as it reads nicer.  I prefer code to read like children's books
> rather than mystery novels.

I think we're reaching the point of diminishing returns here.  I can't
muster a strong feeling either way about your suggestion to add a "len"
variable.

BTW, I purposely didn't use a "for" loop at the end (even though I
usually like them) because I wanted to keep it prominent that path is
being updated to the value of slash.  Putting that assignment in a for
loop makes it easy to overlook because it puts "path" in the spot that
usually holds an inconsequential iteration variable.

YMMV.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/