Greg Stein <gstein@lyra.org> writes:
> > typedef svn_error_t *(*svn_ra_close_commit_func_t) (void *close_baton,
> > svn_stringbuf_t *path,
> > + svn_boolean_t recurse,
> > svn_revnum_t new_rev);
>
> Why did you introduce enum svn_recurse_kind, and then use a boolean here?
>
> Same for the bump function.
Because the svn_recurse_kind is used as the *value* of the hash of
paths. The value tells us whether a committed path needs to be bumped
recursively or not.
Now, in an ideal world, I would have stored an svn_boolean_t as the
value instead, but we can't store 0 as a hash value, due to apr's hash
implementation. That's the only reason svn_recurse_kind exists: to
store 1/2 instead of 0/1. Just because this kind exists, I don't feel
we should use it everywhere. An svn_boolean_t is what I really
"mean", so I'd rather that whereever allowed, and use the enum only
when talking about hash values. I'd rather not add more abstractions
to the API declaration. (It matches all other 'recurse' args of other
functions, anyway.)
> > + /* Add PATH to the hash. */
> > + apr_hash_set (paths, path->data, APR_HASH_KEY_STRING, (void *) 1);
>
> You have the length as path->len, so you should use it. APR_HASH_KEY_STRING
> just makes the hash code work harder. The whole point of counted strings is
> to keep the length handy so you don't have to use strlen() all the time.
>
> The APR_HASH_KEY_STRING is handy if you're using a string constant, or you
> just have a char*. But if you already have the length, then use it.
Then we have a big code change to make. I guess there are at least a
dozen or more places in the code where we use this macro with an
svn_stringbuf_t. In fact, I think *every* apr_hash_set uses the
macro; about a year ago you told us to use the macro, and since then
it's been used exclusively and without abandon. But I see now we
should be a bit more thoughtful. :-)
> > + apr_hash_this (hi, &key, &keylen, &val);
> > + child_path = (const char *) key;
>
> That cast isn't needed. void * -> const char * is automatic.
But isn't it more readable? It reminds us that we're getting a string.
>
> > + child_path_str = svn_stringbuf_create (child_path, pool);
>
> Use ncreate() since you have keylen. If you don't want to use keylen, then
> pass NULL to apr_hash_this.
Hmmm, again, we need to go find every instance of apr_hash_this() in
the code. There's not single instance where we don't generate
keylen. And I'm sure that we ignore it most of the time.
> >
> > - apr_hash_this (hi, (void *) &path, &ignored_len, &ignored_val);
> > + apr_hash_this (hi, (void *) &path, &ignored_len, &val);
>
> Pass NULL rather than &ignored_len.
Check.
> > /* Oh yes, the flogging ritual, how could I forget. */
> > path_str.data = path;
> > path_str.len = strlen (path);
>
> Woah. That is an invalid svn_stringbuf_t. It doesn't have a pool associated
> with it. Either make a real svn_stringbuf_t, or change things to use
> svn_string_t (allowing you to do the above logic).
Now I'm scared; this code is copied and pasted from other parts. It
probably vastly predates the "stringbuf internal pool" change. Hmmm.
>
> merge.c does not use spaces before the paren. Please retain the local style.
>
Check.
> > + do {
> > + svn_path_remove_component (parent_path, svn_path_local_style);
> > + if (r = (enum svn_recurse_kind) apr_hash_get (valid_targets,
> > + parent_path->data,
> > + APR_HASH_KEY_STRING))
>
> "if (x = y)" is confusing. Some compilers will warn about, and every reader
> is going to be suspicioues. For these constructs, make the test explicit
> with "if ((x = y) != 0)"
Good style point. Nobody's ever said that to me, but now I know why
you do things this way.
>
> > + if (r == svn_recursive)
> > + return TRUE;
>
> But since you test for an explicit value here, just drop the if statement
> above. Assign straight to 'r' and then test it once.
Check.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:48 2006