[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r25554 - trunk/subversion/libsvn_wc

From: Malcolm Rowe <malcolm-svn-dev_at_farside.org.uk>
Date: 2007-06-27 17:48:35 CEST

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)

> "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..."

> "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?

> > (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

> Hopefully this is not a bikedshed issue!

Oh, I'm sure it is :-) I should probably surround the entirety of this
mail in <BIKESHED><IMHO> elements...


  • application/pgp-signature attachment: stored
Received on Wed Jun 27 17:48:45 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.