On Thu, 2008-11-13 at 04:47 -0800, Greg Stein wrote:
> Why not just change the function signature and the callers?
The thought crossed my mind, but a code change takes more care and
testing, and we're trying to stabilise and concentrate on bugs. That's
why I didn't do it.
I take your point about being consistent with the way "loggy" functions
take a pointer-to-pointer and allocate if necessary. You pointed this
out to me before about some semi-public APIs and I agreed and changed
them. However, in this file it is common for local functions to take a
pointer-to-existing-buffer, and the callers all have a log accumulator
already (one of them appears to create it specifically for this
purpose).
- Julian
> On Thu, Nov 13, 2008 at 3:57 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> > 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
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
---------------------------------------------------------------------
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 14:31:39 CET