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)