Why not just change the function signature and the callers?
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
Received on 2008-11-13 13:47:57 CET