On Fri, 2010-04-02, Daniel Näslund wrote:
> On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
> > On Tue, 2010-03-30, Daniel Näslund wrote:
> > > > /* An svn_wc__node_found_func_t callback function for analyzing the status
> > > > - * of nodes */
> > > > + * of nodes. Optimized to avoid text compares and unneccessary checks of
> > > > + * already set values. */
> >
> > Be more specific: Which nodes does it analyze? How does it return the
> > result? What kinds of status can it report? (A reference to somewhere
> > else is fine, if the details are already explained somewhere else.)
> > Then the "Optimised ..." sentence should make more sense.
>
> I've clarified exactly how the function is optimized and what parameters
> it takes. A doc comment should say what a func does, rather than how it
> does it. In that sence, my comment is a bit off. Still, someone reading
> a static func is surely going to look at the implementation as well.
When I asked for a more specific doc string, I was under the impression
that you had written this function originally (in an earlier patch). I
have just looked again and seen that you didn't, you just converted it
from a different older function. If I had noticed that originally, I
would not have asked you for so much.
The new version is better, thank you.
> > > > @@ -71,24 +71,36 @@
> > > > wb->result->sparse_checkout = TRUE;
> > > > return SVN_NO_ERROR;
> > > > }
> > > > + else if (status == svn_wc__db_status_not_present)
> > > > + {
> > > > + return SVN_NO_ERROR;
> > > > + }
> > > > + else if (status == svn_wc__db_status_added
> > > > + || status == svn_wc__db_status_obstructed_add
> > > > + || status == svn_wc__db_status_deleted
> > > > + || status == svn_wc__db_status_obstructed_delete)
> > > > + {
> > > > + wb->result->modified = TRUE;
> > > > + }
> >
> > (Minor nit: "else" is redundant after a "return". I don't particularly
> > mind, but somebody objected to them the other day.)
>
> I'm insisting on my if-else construction. Just a matter of personal
> preferences. I think the if-else construction ties the function that
> part together and makes it more readable.
Ah, I didn't remember that it was *you* having that discussion a week or
two ago, I just remembered that somebody said it to somebody. Yes,
that's fine.
> Can I get your +1 for this?
I see it's already committed, and yes, +1 from me too. Thanks.
- Julian
Received on 2010-04-06 13:09:53 CEST