SVN type concerns

[cm...@collab.net]

While working in the FS code, I noticed that there are several
question functions with return values of 'int' as opposed to the
(IMHO) more semantically accurate 'svn_boolean_t'.  Any comments about
this sort of thing?  I'm tempted to blast through there and change
them, but am willing to live with it if there is a sound reason behind
the status quo.

Coming from a Windows Hungarian-notation world where variable names
tell you everything you want to know about types, and into a world
where types themselves take on more emphasis (and variable names are
rather mundane), I'm not qualified to make a value judgment on this
topic.

Re: SVN type concerns

[Kevin Pilch-Bisson <ke...@pilch-bisson.net>]

I'm +1 on keeping and using svn_boolean_t and TRUE and false, for all
the same reasons Greg points out below.

On Thu, Feb 22, 2001 at 09:00:07PM -0800, Greg Stein wrote:
> On Thu, Feb 22, 2001 at 01:34:18PM -0600, Karl Fogel wrote:
> >...
> > I definitely find that svn_boolean_t makes code more readable for me;
> > I think cmpilato and a few others feel the same way.
> 
> This is because you see "int" and wonder about the domain of legal values.
> Can you use -3? How about 42?
> 
> If you see svn_boolean_t or bool or boolean, you *know* the domain is
> comprised of 0 and 1. Anything else is "illegal".
> 
> On the other side of the boolean wall, when you see a function used like
> this:
> 
>     call_some_func ("your mom", "your brother", "don't want to know", 1);
> 
> What the hell does that 1 mean? What if I passed 7?
> 
> If the last line said TRUE, then I know it is representing a boolean value.
> 
> Sure, sure... "documentation saves all". Not when you're reading through
> pages of code. Relying on the doc to pull yourself out of this situation is
> a bit scary. Want an excellent example case? Consider the other day when Jim
> put -1 into the code. Sure, the doc *over in that file there* said what the
> hell it meant, but I wasn't reading that file. I just saw -1 and wondered
> WTF that meant. Using a symbol such as SVN_FS__UNUSED_REVNUM would have been
> immediately obvious.
> 
> TRUE and FALSE play the same game: *local* documentation for the task at
> hand.
> 
> 
> svn_boolean_t is great for documenting the constraints of the particular
> lvalue. TRUE/FALSE are great for documenting how a particular value is going
> to be interpreted.
> 
> Documentation *elsewhere* can be helpful, but should never be considered the
> sole source. Local doc is always an improvement.
> 
> 
> Note that the use of svn_boolean_t and svn_revnum_t are analogous. Local doc
> about a particular type: its meaning, its domain, where it can be passed
> around (e.g. you don't pass an svn_revnum_t to a func taking a long int:
> there is probably a mistake somewhere (possibly as mundane as a mistype on
> the func decl)). TRUE and SVN_INVALID_REVNUM are also analogous.
> 
> Cheers,
> -g
> 
> -- 
> Greg Stein, http://www.lyra.org/

-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson                    http://www.pilch-bisson.net
     "Historically speaking, the presences of wheels in Unix
     has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Re: SVN type concerns

[Greg Stein <gs...@lyra.org>]

On Thu, Feb 22, 2001 at 01:34:18PM -0600, Karl Fogel wrote:
>...
> I definitely find that svn_boolean_t makes code more readable for me;
> I think cmpilato and a few others feel the same way.

This is because you see "int" and wonder about the domain of legal values.
Can you use -3? How about 42?

If you see svn_boolean_t or bool or boolean, you *know* the domain is
comprised of 0 and 1. Anything else is "illegal".

On the other side of the boolean wall, when you see a function used like
this:

    call_some_func ("your mom", "your brother", "don't want to know", 1);

What the hell does that 1 mean? What if I passed 7?

If the last line said TRUE, then I know it is representing a boolean value.

Sure, sure... "documentation saves all". Not when you're reading through
pages of code. Relying on the doc to pull yourself out of this situation is
a bit scary. Want an excellent example case? Consider the other day when Jim
put -1 into the code. Sure, the doc *over in that file there* said what the
hell it meant, but I wasn't reading that file. I just saw -1 and wondered
WTF that meant. Using a symbol such as SVN_FS__UNUSED_REVNUM would have been
immediately obvious.

TRUE and FALSE play the same game: *local* documentation for the task at
hand.


svn_boolean_t is great for documenting the constraints of the particular
lvalue. TRUE/FALSE are great for documenting how a particular value is going
to be interpreted.

Documentation *elsewhere* can be helpful, but should never be considered the
sole source. Local doc is always an improvement.


Note that the use of svn_boolean_t and svn_revnum_t are analogous. Local doc
about a particular type: its meaning, its domain, where it can be passed
around (e.g. you don't pass an svn_revnum_t to a func taking a long int:
there is probably a mistake somewhere (possibly as mundane as a mistype on
the func decl)). TRUE and SVN_INVALID_REVNUM are also analogous.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Re: SVN type concerns

[Jim Blandy <ji...@zwingli.cygnus.com>]

Karl Fogel <kf...@galois.ch.collab.net> writes:
> Consistency in this relatively minor matter is not very important,
> IMHO not nearly important enough to require resolution or a vote or
> something.  Let's just do this: Jim use ints, and we'll use
> svn_boolean_t's.  A person tends to spend the most time reading their
> own code, so this works out well.

That sounds like chaos.  Let's be consistent and use svn_boolean_t
throughout the code.

Re: SVN type concerns

[Jim Blandy <ji...@zwingli.cygnus.com>]

cmpilato@collab.net writes:
> Karl Fogel <kf...@galois.ch.collab.net> writes:
> > I'd like to hear others' thoughts on this.  For what it's worth, I'm
> > +0 on getting rid of svn_boolean_t.  It does give a readability bonus
> > imho, but it's not worth a whole new type.
> 
> I'm -0 on getting rid of svn_boolean_t.  If it stays, though, +1 on
> typedef'ing svn_boolean_t to 'unsigned char' just to piss Jim off. ;-)

This is the correct attitude.

Re: SVN type concerns

[cm...@collab.net]

Karl Fogel <kf...@galois.ch.collab.net> writes:

> I'd like to hear others' thoughts on this.  For what it's worth, I'm
> +0 on getting rid of svn_boolean_t.  It does give a readability bonus
> imho, but it's not worth a whole new type.

I'm -0 on getting rid of svn_boolean_t.  If it stays, though, +1 on
typedef'ing svn_boolean_t to 'unsigned char' just to piss Jim off. ;-)

Re: SVN type concerns

[Karl Fogel <kf...@galois.collab.net>]

Ben Collins-Sussman <su...@newton.ch.collab.net> writes:
> I'm +1 on killing svn_boolean_t altogether.
> 
> But -- I don't want to get rid of our TRUE and FALSE defines.  I think
> that "return FALSE" is easier to read than "return 0", for the same
> reason that I think "return SVN_NO_ERROR" is clearer than "return 0".
> Zero can have different meanings in different contexts.

That's fine -- I think that can really be left to personal preference,
since it's not actually part of the public interface.  (Well, it might
be part of the documentation, but people usually read "return
non-zero" and "return true" to mean the same thing.)

Re: SVN type concerns

[Ben Collins-Sussman <su...@newton.ch.collab.net>]

Karl Fogel <kf...@galois.ch.collab.net> writes:

> I'd like to hear others' thoughts on this.  For what it's worth, I'm
> +0 on getting rid of svn_boolean_t.  It does give a readability bonus
> imho, but it's not worth a whole new type.

I'm +1 on killing svn_boolean_t altogether.

But -- I don't want to get rid of our TRUE and FALSE defines.  I think
that "return FALSE" is easier to read than "return 0", for the same
reason that I think "return SVN_NO_ERROR" is clearer than "return 0".
Zero can have different meanings in different contexts.

Re: SVN type concerns

[Karl Fogel <kf...@galois.collab.net>]

Karl Fogel <kf...@galois.collab.net> writes:
> Consistency in this relatively minor matter is not very important,
> IMHO not nearly important enough to require resolution or a vote or
> something.  Let's just do this: Jim use ints, and we'll use
> svn_boolean_t's.  A person tends to spend the most time reading their
> own code, so this works out well.
> 
> Of course, if you have the sense that you're making changes in an area
> of code that is more or less "owned" by someone else, try to stick to
> their style.  For example, if I were to edit node-rev.c, I wouldn't
> start using svn_boolean_t in code that otherwise uses int.  But
> likewise, I'd expect to see svn_boolean_t in editor.c.

Oh, I'm sorry, my above proposal sidesteps the real issue: that there
are certain public interfaces which *return* an int to signify a
boolean result.

There I do think it's inconsistent that some Subversion libraries
return svn_boolean_t, and others return int.  We should either decide
to get rid of svn_boolean_t entirely, or use it in the fs interfaces
just as we use it elsewhere.

(Either way would be a trivial change, and I'll certainly volunteer to
do it, whatever we decide.)

I'd like to hear others' thoughts on this.  For what it's worth, I'm
+0 on getting rid of svn_boolean_t.  It does give a readability bonus
imho, but it's not worth a whole new type.

(Yes, this could be seen as a reversal from something I argued
earlier, but if you point that out I'll ping flood you.)

-K

Re: SVN type concerns

[Karl Fogel <kf...@galois.collab.net>]

Jim Blandy <ji...@zwingli.cygnus.com> writes:
> > While working in the FS code, I noticed that there are several
> > question functions with return values of 'int' as opposed to the
> > (IMHO) more semantically accurate 'svn_boolean_t'.  Any comments about
> > this sort of thing?  I'm tempted to blast through there and change
> > them, but am willing to live with it if there is a sound reason behind
> > the status quo.
> 
> It's all the functions, in fact.  I just said to Karl on the phone,
> essentially, "The reason behind the status quo is that y'all are
> right, and I'm wrong, but I will revert any change that introduces
> svn_boolean_t into the filesystem, because I am an asshole."

:-)

> [...]
>
> That explained, I'm going to back off, and say that, as long as folks
> understand that I honestly believe it makes the code less readable, if
> you all really think that svn_boolean_t is the best way to go, I won't
> fight it any more.

I definitely find that svn_boolean_t makes code more readable for me;
I think cmpilato and a few others feel the same way.

Perhaps this would logically mean that Jim should use svn_boolean_t in
his code, and we should use ints, because after all, readability is
most important for those who aren't intimately familiar with the code.

However, that would be silly. :-)

Consistency in this relatively minor matter is not very important,
IMHO not nearly important enough to require resolution or a vote or
something.  Let's just do this: Jim use ints, and we'll use
svn_boolean_t's.  A person tends to spend the most time reading their
own code, so this works out well.

Of course, if you have the sense that you're making changes in an area
of code that is more or less "owned" by someone else, try to stick to
their style.  For example, if I were to edit node-rev.c, I wouldn't
start using svn_boolean_t in code that otherwise uses int.  But
likewise, I'd expect to see svn_boolean_t in editor.c.

-K

Re: SVN type concerns

[Jim Blandy <ji...@zwingli.cygnus.com>]

cmpilato@collab.net writes:
> While working in the FS code, I noticed that there are several
> question functions with return values of 'int' as opposed to the
> (IMHO) more semantically accurate 'svn_boolean_t'.  Any comments about
> this sort of thing?  I'm tempted to blast through there and change
> them, but am willing to live with it if there is a sound reason behind
> the status quo.

It's all the functions, in fact.  I just said to Karl on the phone,
essentially, "The reason behind the status quo is that y'all are
right, and I'm wrong, but I will revert any change that introduces
svn_boolean_t into the filesystem, because I am an asshole."

Here is the honest truth: I find svn_boolean_t, TRUE, and FALSE harder
to read than `int', 1, and 0.  You can't use C without knowing that
C's boolean values are really integers; these macros simply introduce
another layer of indirection between the reader and the code's
meaning.  Practically every package out there introduces its own
boolean type, and defines its own named constants for true and false.
So when I'm reading code, I'm internally converting all these slightly
different forms into the canonical form: zero and non-zero.

When I read code which just uses zero and non-zero directly, I breathe
a sigh of relief.  It's like the difference between using good old
char *'s for strings and some opaque datatype --- instead of having to
go find the accessors and make sure they do what you expect, you can
just say "ahhhh" and know what's going on.

svn_boolean_t does provide the benefit of being a form of
documentation for arguments.  But the function's description should be
telling you that anyway.  The fact that something's boolean doesn't
tell me what it does, or the sense of the value (what true means,
vs. what false means) --- I need to check the docs.

At an emotional level, svn_boolean_t smacks of an impulsive urge to
abstract and name "cool" things.  It seems amateurish, like people
still so amazed by the filesystem that they choose filenames ending in
".file".  But I don't expect that to persuade anyone.

That explained, I'm going to back off, and say that, as long as folks
understand that I honestly believe it makes the code less readable, if
you all really think that svn_boolean_t is the best way to go, I won't
fight it any more.