On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> On Thu, 2010-11-04 at 11:51 +0000, Julian Foad 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. :-/
>
> Having said all that, +1 on removing the gratuitous inconsistency by
> applying this patch.
>
> Committed r1030909.
Gah. Can we please wait a little bit longer on this kind of stuff to
allow people in other timezones a chance to weigh in?
-Hyrum (who's own opinion on the matter is forthcoming)
Received on 2010-11-04 13:45:42 CET