Nice!
A few minor comments inline below.
On Mon, 27 Nov 2006, lgo@tigris.org wrote:
...
> Improve the performance of svn_path_is_root by replacing the call to apr_filepath_root
> with a custom implementation. Because the new implementation doesn't require a pool,
> it's removed from the signature and all callers are updated.
>
> * subversion/libsvn_subr/path.c
> (svn_path_is_root): new custom implementation, remove pool from signature.
> (is_canonical, svn_path_join, svn_path_join_many,
> previous_segment, svn_path_basename, get_path_ancestor_length,
> svn_path_is_child): remove the pool from the call to svn_path_is_root.
>
> * subversion/include/svn_path.h
> (svn_path_is_root): remove the pool parameter from the function signature
...
> --- trunk/subversion/include/svn_path.h (original)
> +++ trunk/subversion/include/svn_path.h Mon Nov 27 15:52:51 2006
> @@ -181,8 +181,7 @@
> * hand, amongst which '/' on all platforms or 'X:/', '\\\\?\\X:/',
> * '\\\\.\\..', '\\\\server\\share' on Windows.
> */
> -svn_boolean_t svn_path_is_root(const char *path, apr_size_t len,
> - apr_pool_t *pool);
> +svn_boolean_t svn_path_is_root(const char *path, apr_size_t len);
Because there was no @since tag on this API, I couldn't easily tell
whether this was a backwards compatible change. I've added an @since
tag in r22479. :)
...
> --- trunk/subversion/libsvn_subr/path.c (original)
> +++ trunk/subversion/libsvn_subr/path.c Mon Nov 27 15:52:51 2006
...
> -svn_path_is_root(const char *path, apr_size_t len, apr_pool_t *pool)
> +svn_path_is_root(const char *path, apr_size_t len)
> {
> - const char *root_path = NULL;
> - apr_status_t status;
> - apr_pool_t *strpool = (pool) ? pool : svn_pool_create(NULL);
> - const char *rel_path = apr_pstrmemdup(strpool, path, len);
> - const char *rel_path_apr;
> - svn_boolean_t result = FALSE;
> -
> - /* svn_path_cstring_from_utf8 will create a copy of path.
> -
> - It should be safe to convert this error to a false return value. An error
> - in this case would indicate that the path isn't encoded in UTF-8, which
> - will cause problems elsewhere, anyway. */
> - svn_error_t *err = svn_path_cstring_from_utf8(&rel_path_apr, rel_path,
> - strpool);
> - if (err)
> - {
> - svn_error_clear(err);
> - goto cleanup;
> - }
> -
> - status = apr_filepath_root(&root_path, &rel_path_apr, 0, strpool);
Please add an inline comment to this code explaining why we don't use
apr_filepath_root() (instead using our own custom implementation).
...
> + /* path is root if it's equal to '/' */
> + if (len == 1 && path[0] == '/')
> + return TRUE;
> +
> +#if defined(WIN32) || defined(__CYGWIN__)
> + /* On Windows and Cygwin, 'H:' or 'H:/' (where 'H' is any letter)
> + are also root paths */
> + if ((len == 2 || len == 3) &&
> + (path[1] == ':') &&
> + ((path[0] >= 'A' && path[0] <= 'Z') ||
> + (path[0] >= 'a' && path[0] <= 'z')) &&
> + (len == 2 || (path[2] == '/' && len == 3))
> + )
> + return TRUE;
Subversion code typically seems to close this kind of conditional on
the previous line (e.g. move the paren).
> +
> + /* On Windows and Cygwin, both //drive and //drive//share are root paths */
> + if (len >= 2 && path[0] == '/' && path[1] == '/' && path[len - 1]!='/')
> {
...
> + int segments = 0;
> + int i;
> + for (i = len; i >=2; i--)
^
Some whitespace around the right side of the ">=" operator would be
more comprehensible.
> + {
> + if (path[i] == '/')
> + {
> + segments ++;
> + if (segments > 1)
> + return FALSE;
> + }
> + }
> + if (segments <= 1)
> + return TRUE;
> + else
> + return FALSE;
> + }
> +#endif /* WIN32 or Cygwin */
> +
> + return FALSE;
> }
...
> --- trunk/subversion/libsvn_wc/lock.c (original)
> +++ trunk/subversion/libsvn_wc/lock.c Mon Nov 27 15:52:51 2006
...
> @@ -1588,3 +1588,4 @@
>
>
>
> +
Spurious change here.
- application/pgp-signature attachment: stored
Received on Tue Nov 28 18:24:55 2006