Re: svn commit: r18486 - trunk/subversion/libsvn_ra_serf

On 2/16/06, Daniel Rall <dlr@collab.net> wrote:
> Comment doesn't add a lot of additional value. I recommend either
> removing it, or being a bit more verbose:

I'll remove the comment.

> > @@ -283,6 +303,7 @@
> > new_state->info->dir->parent_dir->ref_count++;
> >
> > new_state->info->dir->props = apr_hash_make(new_state->info->pool);
> > + new_state->info->dir->ns_list = new_state->info->dir->ns_list;
>
> Why point ns_list back at itself? (I can recommend a good
> psychiatrist. ;) Should it actually reference a member of ctx?

Good catch. It should refer to it's parent ns_list as the parent's
pool has at least as long a lifetime and we may be able to piggy back
on its ns_list allocations (maybe saving us some allocations of our
own).

> Need a #define for the magic number?

I'm still trying to determine what the 'right' number is. 2KB is
certainly not large enough as that tends to just make us repeat reads
(i.e. we typically have more than 2KB available when we go to read).

So, I expect the magic number to change as I quantify this loop's
performance more...

> > + /* We need to move the prop_ns, prop_name, and prop_val into the
> > + * same lifetime as the dir->pool.
> > + */
> > + ns_t *ns, *ns_name_match;
> > + int found = 0;
>
> Why not use a svn_boolean_t here to represent the flag, rather than an
> int?

Again, some of this XML code may end up in serf which won't have a boolean flag.

> TRUE instead of 1.

I'm really hesitant to use TRUE by itself to equal 1 (for a
comparison, sure? but, not ever by itself). while(1729) would be the
more Subversion-y way to write while(1). =)

> > + if (strcmp(cur_ns->namespace, tmp_attrs[0]+6) == 0)
>
> Some whitespace around that '+' operator would make this more easily
> readable.

Sure. -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 17 08:11:49 2006