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.
-Hyrum
Received on 2010-11-04 13:48:04 CET