On Thu, 2010-11-04 at 07:45 -0500, Hyrum K. Wright wrote:
> 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)
As regards your concern that they are subtly different: yes, I share the
concern. I think as a generality we are unaccustomed to tri-state
variables and are likely to make mistakes. So maybe making them
incompatible is in fact better.
My take on this was: whether we decide to make it intentionally
compatible or intentionally incompatible, the current (before I
committed that) definition was, IMO, gratuitously confusing. I thought
that this commit was an OK step whatever the final decision.
But I guess committing that has implicitly supported the view that they
should be intentionally compatible.
I'll *try* to wait a bit longer on things.
- Julian
Received on 2010-11-04 16:17:09 CET