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.f...@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