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..."
(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?
> > (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).
> 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...
Regards,
Malcolm
- application/pgp-signature attachment: stored
Received on Wed Jun 27 17:48:45 2007