> -----Original Message-----
> From: Malcolm Rowe [mailto:malcolm-svn-dev@farside.org.uk]
> Sent: Wednesday, June 27, 2007 11:49 AM
> To: Paul Burba
> Cc: dev@subversion.tigris.org
> Subject: Re: svn commit: r25554 - trunk/subversion/libsvn_wc
>
> On Wed, Jun 27, 2007 at 08:15:34AM -0700, Paul Burba wrote:
> > > In general, our doc-strings should just say something like "Set
> > > *switched to true if ...; otherwise, set *switched to false", and
> > > not worry about whether we're using TRUE specifically.
> >
> > In the past I've gotten advice in different directions on
> this question.
> > Certainly we are inconsistent in our docstrings regarding
> > "svn_boolen_t *pred" type arguments.
> >
> > We commonly use
> >
> > "set *pred to true"
> (that's my preference; it doesn't imply that the caller is
> free to check against specific values)
I agree, and tweaked the docstring for svn_wc__path_switched() to follow
that style.
> > "set *pred to TRUE"
> > "set *pred to @c TRUE"
> (the former's probably intended to be the latter)
>
> > But we also spice things up with:
> >
> > "set @a *changed_p to 1..."
> (bad)
>
> > "Set @a *modified_p to non-zero"
> (okay, I guess)
>
> > "return the answers in @a text_conflicted_p and @a
> prop_conflicted_p."
> (fine, assuming the meaning of the arguments is obvious).
>
> > Implementations are also varied, some setting TRUE/FALSE explicitly
> > some using the result of a conditional statement.
>
> I'm really not bothered about the implementations as much as
> what the callers should be doing. Since you switched an
> implementation to match the API promise, I wondered if you'd
> found a caller that was checking against TRUE directly?
Sorry, no callers doing that. I just got it in my head to follow the
API promise, without realizing that promise is not ideal.
> > > (and we obviously shouldn't be testing again TRUE directly).
> > >
> > > > - *switched = strcmp(parent_child_url, entry->url);
> > > > + *switched = strcmp(parent_child_url, entry->url) ==
> 0 ? FALSE :
> > > > + TRUE;
> > >
> > > (Incidentally, I find both of those forms really hard to parse.
> > > That might just be me, however).
> >
> > Heh, ok, I admit it, I'm in love with the ternary operator. But if
> > there is anything even approaching general agreement that the above
> > examples hinder code readability, I'm more than happy to
> use something
> > like this from now on:
> >
> > if (strcmp(parent_child_url, entry->url))
> > *switched = TRUE;
> > else
> > *switched = FALSE;
> >
>
> That's certainly clearer than what's there now, but I was
> more concerned about whether we had some need to make this
> change over and above meeting the API description (which I
> think is too strict, as mentioned above).
No need for the ternary operator as many pointed out. I used it
originally because I was thinking that an equality expression in C might
return zero or *non-zero* rather than always 1, I should have checked my
C standard book :-\
In r25555 went with the glasser/cmpilato suggested:
*switched = strcmp(parent_child_url, entry->url) != 0;
Thanks all,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jun 27 18:28:18 2007