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.
Thanks.
- Julian
Received on 2010-11-04 13:43:47 CET