On Thu, 2008-11-13 at 11:55 +0100, Stefan Sperling wrote:
> On Wed, Nov 12, 2008 at 08:31:15PM -0800, neels_at_tigris.org wrote:
> > Author: neels
> > Date: Wed Nov 12 20:31:15 2008
> > New Revision: 34165
> >
> > Log:
> > * subversion/libsvn_wc/update_editor.c
> > (check_tree_conflict): Add an assertion with comment.
> >
> > Modified:
> > trunk/subversion/libsvn_wc/update_editor.c
> >
> > Modified: trunk/subversion/libsvn_wc/update_editor.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/update_editor.c?pathrev=34165&r1=34164&r2=34165
> > ==============================================================================
> > --- trunk/subversion/libsvn_wc/update_editor.c Wed Nov 12 19:54:50 2008 (r34164)
> > +++ trunk/subversion/libsvn_wc/update_editor.c Wed Nov 12 20:31:15 2008 (r34165)
> > @@ -1466,6 +1466,14 @@ check_tree_conflict(svn_wc_conflict_desc
> > conflict->action = action;
> > conflict->reason = reason;
> >
> > + /* This function recevies only an svn_stringbuf_t*, but passes an
>
> Should say "receives"
>
> > + * svn_stringbuf_t** into below function. Changes by that function
> > + * to the svn_stringbuf_t* don't propagate back to the caller of
> > + * check_tree_conflict(). However, svn_wc__loggy_add_tree_conflict()
> > + * will only change the svn_stringbuf_t* if it is NULL, so let's
> > + * make sure a log_accum is already in place. */
> > + SVN_ERR_ASSERT(log_accum != NULL);
> > +
> > SVN_ERR(svn_wc__loggy_add_tree_conflict(&log_accum, conflict,
> > parent_adm_access, pool));
> The description mixes a type (svn_stringbuf_t) with a variable name
> (log_accum) to refer to the same thing.
I didn't think that made it any harder to understand, but then I'm
fairly familiar with it already.
> Also, stating the called function's name instead of "below function"
> makes this comment a tad more resistant against time, and consistently
> referring to check_tree_conflict as "this function" and makes it easier
> to follow, too, I think.
>
> /* This function receives log_accum as an svn_stringbuf_t*,
> * but passes it as an svn_stringbuf_t** into
> * svn_wc__loggy_add_tree_conflict().
> * Changes made by svn_wc__loggy_add_tree_conflict() to the
> * svn_stringbuf_t* don't propagate back to the caller of this
> * function. However, svn_wc__loggy_add_tree_conflict() will only
> * change the svn_stringbuf_t* if it is NULL, so let's make sure a
> * log_accum is already in place. */
> SVN_ERR_ASSERT(log_accum != NULL);
>
> What do you think?
It's perhaps clearer in its references, but it still seems to bury the
main point in too many details.
I changed it in r34168 to say:
/* Ensure 'log_accum' is non-null. svn_wc__loggy_add_tree_conflict()
* would otherwise quietly set it to point to a newly allocated buffer
* but we have no way to propagate that back to our caller. */
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-13 12:58:07 CET