On 04.11.2010 13:47, Hyrum K. Wright wrote:
> On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad<julian.foad_at_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.
Received on 2010-11-07 14:22:02 CET