On Mon, Nov 12, 2001 at 11:18:35AM -0600, Ben Collins-Sussman wrote:
> 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.
You put svn_recurse_kind into svn_types.h. Thus, I would expect it to be
used everywhere that recursion is an issue. If you intended it only to be
used for the hash, then it should have been called
"svn_only_use_this_as_a_hash_value_recursion_thingy"
Why the arbitrary restriction to using it only for the hash value? I see no
rationale for that limitation.
> 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.)
And we're in the midst of considering that recursion would be expanded into
a Depth-like thing: no recurse, children only, full recurse. This enum is
exactly the path towards that.
Using a boolean in some places and an enum in others is going to cause
problems in understanding the API. "Why are these different?" "Whoops! I
can't pass that value to that one!" Just think what happens when people pass
an enum value to a boolean arg, or vice versa. They are type-copmatible
after all...
Separate types is opening us up for bugs :-(
> > > + /* 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. :-)
I said to use that rather than calling strlen() in the caller. A lot of code
was doing stuff like:
apr_hash_set(foo, "this is a long key", strlen("this is a lng key"), blah);
Oops. Typo. Or sometimes there was a symbolic constant, or a variable, or
whatever. The point is that using APR_HASH_KEY_STRING lets the hash
implementation optimize how it will handle the key, rather than forcing the
strlen in the caller.
But if you *have* the length, then it should be used.
> > > + 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.
I never find casts to be more readable. They will also clutter things up
when we go and try to find invalid casts in the future (e.g. I'd like to be
able to find places where we've casted around cost problems).
The name and type of child_path provide enough information. We don't litter
the code with casts just to keep track of types.
> > > + 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.
There are quite a few places where NULL is passed to the keylen.
>...
> > > /* 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.
Anywhere it exists is a problem. :-(
Switching to svn_string_t will help.
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 Sat Oct 21 14:36:48 2006