On 04.11.2010 13:47, Hyrum K. Wright wrote:
On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad<julian.f...@wandisco.com> wrote:
On Thu, 2010-11-04, Stefan Fuhrmann wrote:
Hi there,
after stumbling twice over this issue, I ran grep
and found that the current usage of svn_tristate_t
does not depend on the actual numerical values
used for its states.
Therefore, I propose to define svn_tristate_false
equal to FALSE and svn_tristate_true equal to
TRUE. That way, we can compare booleans with
tristates and assign booleans to tristates. Without
that, we need to write code like
if ((get_some_bool() ? svn_tristate_true : svn_tristate_false) ==
get_a_tristate())
...
Also, the following will compile without warning but
requires the patch to do what was intended:
// FALSE becomes "unknown", TRUE becomes "false"
svn_tristate_t my_tristate = get_some_bool();
-- Stefan^2.
[[[
Make svn_tristate_t mainly compatible to svn_boolean_t
by making its numerical values match the ones used for
svn_boolean_t.
* subversion/include/svn_types.h
(svn_tristate_t): define *_true and *_false in terms of
the svn_boolean_t values TRUE and FALSE, respectively.
]]]
It seems sensible to have the set of values either match where they
overlap (like you're suggesting), or be totally incompatible (e.g. the
set of tristate values being 2/3/4, or the tristate data type being
incompatible with "int").
If the values match, then there is no need for the enumerators
"svn_tristate_false" and "svn_tristate_true", since "FALSE" and "TRUE"
can be used instead. If the idea is that these values are compatible,
then we *should* delete the "svn_tristate_true/false" names and use the
existing names for them, and not have the ambiguity of which name to
use.
As Bert said, it's not always safe to assign or compare what we call a
"Boolean" value directly with an svn_tristate_t value, because we can't
always guarantee that "true" is represented by "1". But often it would
be safe. :-/
I don't know how comfortable I am with this change.
Having svn_tristate_t be a superset of svn_boolean_t just means that
folks can/will use the former when they should b using the latter.
Making them interchangable is a Bad Thing because it confuses the
semantics, and doesn't force people to make a decision about which one
to use. "Hmm, which should I use here, a Tristate or a Boolean? Oh
well, it doesn't matter, because Tristate is just a superset of a
Boolean, and they are compatible."
My fears here may be overblown, but there *is* a difference between
and Tristate and a Boolean, and there should be a difference between a
Tristate-true and a Boolean-true. Maintaining the incompatibility in
code between the two is a way to enforce this.
Thanks all for the replies. Here is how I see it plus
a couple of things I discovered in the meantime.
* general consensus: overlapping definitions with
inconsistent meaning are bad.
* enums are always compatible with ints, so we can't
make them incompatible at compile time
That leaves us with two basic options.
(1) Define that a tristate is a "nullable" boolean (.net speak).
To me, this seems to be the way that it is being used
right now. However, one might be tempted to (miss-)use
tristates instead of booleans "just in case". That would
be bad.
(2) Define that tristates and booleans are similar but
fundamentally different. Using values >1 for tristate
values would make them "always true" in boolean
expressions. So, the logic would probably fail early
and the compiler might even generate a warning.
I think there is no big risk associated with neither
of these options. If people feel uncomfortable with (1),
I'm happy to switch to (2).
But I also discovered two issues with the current boolean
type definitions. First, TRUE and FALSE are defined
after I use them in svn_tristate_t which can be fixed easily
and leads directly to the second issue.
We only define TRUE and FALSE in case they have
not been defined, yet. Since we rely on their numerical
values, we should at test for these values in case the
macros are pre-defined (#error if not).
If people agree, I will implement (2) and the fix the other
two issues.
-- Stefan^2.
