On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad <julian.f...@wandisco.com> 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. :-/
I don't know how comfortable I am with this change. Having svn_tristate_t be a superset of svn_boolean_t just means that folks can/will use the former when they should b using the latter. Making them interchangable is a Bad Thing because it confuses the semantics, and doesn't force people to make a decision about which one to use. "Hmm, which should I use here, a Tristate or a Boolean? Oh well, it doesn't matter, because Tristate is just a superset of a Boolean, and they are compatible." My fears here may be overblown, but there *is* a difference between and Tristate and a Boolean, and there should be a difference between a Tristate-true and a Boolean-true. Maintaining the incompatibility in code between the two is a way to enforce this. -Hyrum
