Re: [patch libiberty include]: Add additional helper functions for directory-separator searching

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 8 Mar 2011 11:56:45 +0100
> From: Kai Tietz <ktietz70@googlemail.com>
> 
> +@deftypefn Extension int filename_dirchr (const char *@var{p})
> +
> +The returned value is similar to what @code{strchr} would return for
> +searching for a directory separator.
> +
> +This function does not normalize file name.  However, it does handle
> +the fact that on DOS-like file systems, forward and backward slashes
> +are directory separators.

This is very mysterious.  The documentation should explain how this is
"handled", or else the user will have no choice but to look in the
sources.  And description "by similarity" doesn't help, because this
function is obviously different from strchr in _some_ ways, but you
don't say how.

While at that, explain the problem this solves, or else the raison
d'etre of this function will not be understood.  We do want this
function to be used instead of just strchr, don't we?  For it to be
used, its purpose and advantages should be well understood.

Btw, why do we need filename_dirchr?  The use case for
filename_dirrchr is clear, but in what situations will we need the
other one?

> +  if (!r || (r2 && r2 < r))

Why do you test for r2 being non-NULL?  You are not going to
dereference it in the next comparison, and NULL is comparable as any
other value.

> +@deftypefn Extension int filename_dirrchr (const char *@var{p})
> +
> +The returned value is similar to what @code{strrchr} would return for
> +searching for a directory separator.
> +
> +This function does not normalize file name.  However, it does handle
> +the fact that on DOS-like file systems, forward and backward slashes
> +are directory separators.

Same comments about this doc.

> +  if (!r || (r2 && r2 > r))

And same comment here about testing r2 for non-NULL value.

Please also wait for others to review, as I'm not authorized to
approve the changes.

Thanks for working on this.