Philip Martin <philip@codematters.co.uk> writes:
> The reason to have a function return a boolean is because we expect
> the "error" to occur, we intend to handle it, and we don't want the
> overhead of creating and destroying an error. I don't think that
> applies to this stuff, the error is likely to be irrecoverable. So I
> guess we don't need the boolean function.
We can dispense with the boolean function for now, and just have one
public interface, that returns a specific SVN_ERR_ error if the path
is invalid. If someday we want to ask the question in a
non-error-generating way, we can add the boolean interface then.
> The function name svn_path_error_if_invalid is ugly.
How about svn_path_check_valid()?
I would prefer not to use up the intuitive boolean interface name
"svn_path_is_valid" on an error-generating function. The vast
majority of Subversion uses
svn_foo_is_bar()
for a standard boolean-by-reference form. If we ever want to add a
boolean interface for path validity, we'll want that name available.
Therefore, let's give the error-generating version a different name.
It doesn't have to be "svn_path_error_if_invalid". As long as it's
*not* "svn_path_is_valid", I'm happy :-). "svn_path_check_valid" is
fine, no doubt there are many other possible names.
> I'm guessing we don't need the check_utf8 flag. You have already
> added a new public interface to the existing check-UTF-8 function, so
> all we really need is a new check-control-character-function, one that
> returns an error. Where it is know that the path is UTF-8 just call
> the second, if the path is not known to be UTF-8 call both.
I think it would be good to have a single svn_path_foo() function to
do this. I'm assuming it would look something like this:
svn_error_t *
svn_path_check_valid (const char *path, apr_pool_t *pool)
{
svn_error_t *err;
err = svn_utf_check_cstring_utf8 (path, pool);
if (err && (err->apr_err == SVN_ERR_BLAH_NOT_UTF8))
{
return svn_error_createf
(SVN_ERR_PATH_INVALID,
"Path '%s' is not encoded in UTF8",
svn_some_fuzzy_conversion_function (path, pool));
}
else if (err)
{
return err;
}
err = svn_utf_check_cstring_no_controls (path, pool);
if (err && (err->apr_err == SVN_ERR_BLAH_HAS_CONTROL_CHARS))
{
return svn_error_createf
(SVN_ERR_PATH_INVALID,
"Path '%s' is not encoded in UTF8",
svn_some_fuzzy_conversion_function (path, pool));
}
else if (err)
{
return err;
}
return SVN_NO_ERROR;
}
(Untested code, and the error names are improvised, of course).
True, this ends up resulting in three new public interfaces:
svn_utf_check_cstring_utf8()
svn_utf_check_cstring_no_controls()
svn_path_check_valid()
But the calling sites would become very readable -- they wouldn't even
need comments, because the name of the function will say everything
that needs to be said. That seems like a bargain.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 10 22:56:47 2004