Greg Stein <gstein@lyra.org> writes:
> 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.
The rationale is that I don't want to launch off on a crusade to
upgrade every 'svn_boolean_t recurse' into an 'enum svn_recurse_kind
recurse'. I'm only limiting the usage of this new type temporarily,
to solve exactly the problem it was created for...
> 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.
I have no objection to using 'enum svn_recurse_kind' everywhere
possible. I agree, it's a good road to take. But at the moment, it's
a sidetrack I don't have time to follow. Therefore, for the extreme
short term, I'm only using the enum as a hash-value in one special
case.
I'll add a note to the appropriate 'recurse flags' issue about this,
so whoever is working on that (kpb?) can upgrade all dem flags to the
new enum type.
>
> 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.
Ahhhhhhh, thank you, now I understand.
> > > > + 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.
I don't know what to say, except that it's a matter of style, I guess.
You're older and wiser, so I'll take to heart what you say. :-)
If this style is important to you, then again, go look at nearly
*every* hash loop; we're doing these 'readability' casts all the
time. Maybe we need a 'cleanup code style' issue?
> > 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.
Um, I did a tags-search, and out of 30-something calls to
apr_hash_this, only 7 of them pass NULL as keylen. And most of those
are in mod_dav_svn or libsvn_ra_dav -- proof that you know how to
properly use the call, and the rest of us don't. :-)
I can add this to the code-style-cleanup task.
---------------------------------------------------------------------
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