You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/06/27 14:38:37 UTC
Re: svn commit: r25554 - trunk/subversion/libsvn_wc
On Wed, Jun 27, 2007 at 06:55:20AM -0700, pburba@tigris.org wrote:
> Set TRUE/FALSE *not* non-zero/zero int when testing a WC path's switch status.
>
> * subversion/libsvn_wc/util.c
> (svn_wc__path_switched): Set *switched argument to TRUE or FALSE as per the
> docstring for this function.
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.
(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).
Regards,
Malcolm
Re: svn commit: r25554 - trunk/subversion/libsvn_wc
Posted by "C. Michael Pilato" <cm...@collab.net>.
Paul Burba wrote:
> 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;
>
> Hopefully this is not a bikedshed issue!
There is a bit of a convention in the Subversion codebase (or at least there
was at one time) of always explicitly testing the return value from
strcmp()). Because a "true" return from strcmp actually means, the strings
*don't* compare, we use comparisons with 0 so that '==' and '!=' map to
"strings are equal" and "strings aren't equal", respectively.
I, too, love the ternary operation. So I'd suggest one of these two,
depending on the result of the call on whether we have to explicitly use
FALSE and TRUE values:
*switched = (strcmp(parent_child_url, entry->url) != 0);
*switched = (strcmp(parent_child_url, entry->url) == 0) ? FALSE : TRUE;
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
RE: svn commit: r25554 - trunk/subversion/libsvn_wc
Posted by Paul Burba <pb...@collab.net>.
> -----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
Re: svn commit: r25554 - trunk/subversion/libsvn_wc
Posted by Malcolm Rowe <ma...@farside.org.uk>.
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
RE: svn commit: r25554 - trunk/subversion/libsvn_wc
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Malcolm Rowe [mailto:malcolm-svn-dev@farside.org.uk]
> Sent: Wednesday, June 27, 2007 10:39 AM
> To: dev@subversion.tigris.org; pburba@tigris.org
> Subject: Re: svn commit: r25554 - trunk/subversion/libsvn_wc
>
> On Wed, Jun 27, 2007 at 06:55:20AM -0700, pburba@tigris.org wrote:
> > Set TRUE/FALSE *not* non-zero/zero int when testing a WC
> path's switch status.
> >
> > * subversion/libsvn_wc/util.c
> > (svn_wc__path_switched): Set *switched argument to TRUE
> or FALSE as per the
> > docstring for this function.
>
> 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.
Hi Malcolm,
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"
"set *pred to TRUE"
"set *pred to @c TRUE"
But we also spice things up with:
"set @a *changed_p to 1..."
"Set @a *modified_p to non-zero"
"return the answers in @a text_conflicted_p and @a prop_conflicted_p."
And %DEITY% knows what else...this was just a quick look on my part.
Implementations are also varied, some setting TRUE/FALSE explicitly some
using the result of a conditional statement.
I am not married to any of these, whatver folks agree is the best way is
what I'll use.
> (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;
Hopefully this is not a bikedshed issue!
Thanks,
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org