On Thu, May 02, 2002 at 09:49:06AM -0500, cmpilato@tigris.org wrote:
>...
> Log:
> This affects issue #656.
>
> Begin the work of separating the internal filesystem structures from
> the list/string skeleton library routines. This is a requirement for
> pluggable database backends, and will (I think) assist in the
> conversion to the new NodeId interface.
>...
One overall comment is that I saw a number of uses of apr_pstrndup() when
using apr_pstrmemdup() will be faster (better).
I also really liked the clarification of FOO_key, so it was easier to
understand that only the key was being passed, not the value.
>...
> +++ trunk/subversion/libsvn_fs/reps-table.c Thu May 2 09:48:48 2002
>...
> @@ -99,11 +103,16 @@
> svn_error_t *
> svn_fs__write_rep (svn_fs_t *fs,
> const char *key,
> - skel_t *skel,
> + svn_fs__representation_t *rep,
Should make this 'const'
>...
> @@ -117,7 +126,7 @@
> svn_error_t *
> svn_fs__write_new_rep (const char **key,
> svn_fs_t *fs,
> - skel_t *skel,
> + svn_fs__representation_t *rep,
'const' here, too.
(and propagate these into reps-table.h)
>...
> +++ trunk/subversion/libsvn_fs/dag.c Thu May 2 09:48:48 2002
>...
> @@ -330,14 +331,15 @@
> "revisions" : 0 -> "(revision 3 0.0 ())" */
> {
> /* ### this should be const. we should make parse_skel() take a const */
> - static char rev_skel[] = "(revision 3 0.0 ())";
> + static char rev_skel_str[] = "(revision 3 0.0 ())";
> + svn_fs__revision_t *revision;
> + skel_t *rev_skel;
> svn_revnum_t rev = 0;
> - SVN_ERR (svn_fs__put_rev (&rev, fs,
> - svn_fs__parse_skel (rev_skel,
> - sizeof (rev_skel) - 1,
> - trail->pool),
> - trail));
> -
> +
> + rev_skel = svn_fs__parse_skel (rev_skel_str, strlen (rev_skel_str),
> + trail->pool);
> + SVN_ERR (svn_fs__parse_revision_skel (&revision, rev_skel, trail->pool));
> + SVN_ERR (svn_fs__put_rev (&rev, fs, revision, trail));
There is no reason to construct a skel from a constant string, then parse
that into a revision structure, just to put the structre. Instead, just
manually build the structure and put it.
>...
> +++ trunk/subversion/libsvn_fs/txn-table.c Thu May 2 09:48:48 2002
>...
> static svn_error_t *
> put_txn (svn_fs_t *fs,
> - const char *svn_txn,
> - const svn_fs_id_t *root_id,
> - const svn_fs_id_t *base_root_id,
> - skel_t *proplist,
> + svn_fs__transaction_t *txn,
Should be 'const'
>...
> +svn_fs__create_txn (char **txn_name_p,
> svn_fs_t *fs,
> const svn_fs_id_t *root_id,
> trail_t *trail)
> {
> - char *svn_txn;
> + char *txn_name;
> + svn_fs__transaction_t txn;
> + svn_fs_id_t *id_copy = svn_fs__id_copy (root_id, trail->pool);
It wasn't immediately obvious why the ID was copied. I just realized it is
to get the ID from whereever the caller had it into the trail pool. A
comment might be nice.
(the main point is: it is being copied for pool mgmt purposes, not that it
will be modified)
Further, a reader might also think, "hey. the txn this goes into doesn't
survive the function, so I don't even need a local copy anyways." But
through the trail does, *does* it survive the function? I'm not entirely
clear. In any case, if it does *NOT* survive, then you definitely don't need
to copy the thing.
>...
> + /* Unparse TRANSACTION skel */
> + skel = svn_fs__parse_skel (value.data, value.size, trail->pool);
Um. You're *parsing* the skel, not "Unparse"
>...
> svn_fs__set_txn_base (svn_fs_t *fs,
>...
> + if (! svn_fs__id_eq (txn->base_root_id, new_id))
> + {
> + txn->base_root_id = svn_fs__id_copy (new_id, trail->pool);
Same thing about lifetimes, function survival, adding a comment, etc.
>...
> +++ trunk/subversion/libsvn_fs/reps-strings.c Thu May 2 09:48:49 2002
>...
> +/* Return a `fulltext' representation which references the string
> + STR_KEY, performing allocations in POOL. If MUTABLE is non-zero,
> + make the representation mutable. If non-NULL, STR_KEY will be
> + copied into an allocation of POOL. */
> +static svn_fs__representation_t *
> +make_fulltext_rep (const char *str_key,
>...
> + svn_fs__representation_t *rep = apr_palloc (pool, sizeof (*rep));
> + rep->is_mutable = 1;
> + rep->kind = svn_fs__rep_kind_fulltext;
> + rep->contents.fulltext.string_key = apr_pstrdup (pool, str_key);
The documentation says that STR_KEY might be NULL. Thus, this strdup is
going to fail in that case. Change the doc or the code...
>...
> +delta_string_keys (apr_array_header_t **keys,
> + svn_fs__representation_t *rep,
Should be 'const'
>...
> - if (! rep_is_delta (rep))
> + if (! (rep->kind == svn_fs__rep_kind_delta))
Heh. A lot easier to write that as:
if (rep->kind != svn_fs__rep_kind_delta)
:-)
>...
> + /* Set up a convenience variable. */
> + chunks = rep->contents.delta.chunks;
> + if (! chunks->nelts)
> {
> - num_windows++;
> - window = window->next;
> + *keys = NULL;
> + return SVN_NO_ERROR;
Woah. This changes the semantics of the function. Further, this return of
NULL is not documented in the function's doc comment.
>...
> svn_fs__rep_contents_clear (svn_fs_t *fs,
> - const char *rep,
> + const char *rep_key,
> trail_t *trail)
> {
>...
> - /* Else, clear it. */
> + /* Else, clear it the string the rep has. */
That comment doesn't make any sense :-)
>...
> + /* Allocate a chunk and its window */
> + chunk = apr_palloc (pool, sizeof (*chunk));
> + chunk->offset = ww->text_off;
> + chunk->window = apr_palloc (pool, sizeof (*chunk->window));
> +
> + /* Populate the window */
> + chunk->window->string_key = ww->key;
> + chunk->window->size = ww->text_len;
> + memcpy (&(chunk->window->checksum), digest,
> + (MD5_DIGESTSIZE / sizeof (*digest)));
That division isn't needed. Simply using MD5_DIGESTSIZE should be fine.
>...
> +++ trunk/subversion/libsvn_fs/fs_skels.c Thu May 2 09:48:49 2002
>...
> +svn_error_t *
> +svn_fs__unparse_revision_skel (skel_t **skel_p,
> + svn_fs__revision_t *revision,
Should be 'const'
>...
> +svn_error_t *
> +svn_fs__unparse_transaction_skel (skel_t **skel_p,
> + svn_fs__transaction_t *transaction,
Should be 'const'
>...
> +svn_error_t *
> +svn_fs__unparse_representation_skel (skel_t **skel_p,
> + svn_fs__representation_t *rep,
Should be 'const'
(and the corresponding changes in fs_skels.h for these)
>...
> +++ trunk/subversion/libsvn_fs/fs.h Thu May 2 09:48:55 2002
>...
> +/*** Filesystem Transaction ***/
> +typedef struct
> +{
> + /* id of the root node */
> + svn_fs_id_t *root_id;
> +
> + /* id of the revision root node upon which this txn is base */
> + svn_fs_id_t *base_root_id;
> +
> + /* property list (const char * name, svn_string_t * value) */
> + apr_hash_t *proplist;
svn_fs__create_txn() might set 'proplist' to NULL. This should be doc'd.
>...
> +++ trunk/subversion/libsvn_fs/rev-table.c Thu May 2 09:49:00 2002
>...
> @@ -98,31 +78,50 @@
> /* Handle any other error conditions. */
> SVN_ERR (DB_WRAP (fs, "reading filesystem revision", db_err));
>
> - /* Parse and check the REVISION skel. */
> + /* Unparse REVISION skel. */
> skel = svn_fs__parse_skel (value.data, value.size, trail->pool);
You're parsing, not "Unparse"
>...
> {
> struct revision_prop_args *args = baton;
> -
> - skel_t *skel;
> + svn_fs__revision_t *revision;
> skel_t *proplist;
>
> - SVN_ERR (svn_fs__get_rev (&skel, args->fs, args->rev, trail));
> -
> - /* PROPLIST is the third element of revision skel. */
> - proplist = skel->children->next->next;
> + SVN_ERR (svn_fs__get_rev (&revision, args->fs, args->rev, trail));
> + SVN_ERR (svn_fs__unparse_proplist_skel (&proplist,
> + revision->proplist,
> + trail->pool));
>
> /* Return the results of the generic property getting function. */
> return svn_fs__get_prop (args->value_p,
Ah. I see this was simplified in a later commit. Goodness(tm).
>...
> +++ trunk/subversion/libsvn_fs/rev-table.h Thu May 2 09:49:00 2002
>...
> svn_error_t *svn_fs__put_rev (svn_revnum_t *rev,
> svn_fs_t *fs,
> - skel_t *skel,
> + svn_fs__revision_t *revision,
Should be 'const'
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 14 04:23:14 2002