You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <st...@alice-dsl.de> on 2010/11/03 23:21:07 UTC
[Patch] Make svn_tristate_t compatible with svn_boolean_t
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.
]]]
RE: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
Bert Huijben wrote:
> > -----Original Message-----
> > From: Stefan Fuhrmann [mailto:stefanfuhrmann@alice-dsl.de]
> > 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();
>
> What do you try to get as result here?
>
> I'm not 100% sure if tests like (12 == 12) really always return the defined
> value TRUE
(I'm just addressing the C language facts in this response.)
In C, when an operator such as "==" is defined to yield a Boolean value,
it *always* return 0 for "false" or 1 for "true", never other values for
"true".
However, non-Boolean-valued expressions, such as "flags & 0x40", can be
used where a Boolean value is wanted, such as "if (flags & 0x40)" or
"override || (flags & 0x40)". Any non-zero is regarded as "true".
> and we most likely have other functions that return just some !=
> 0 value as svn_boolean_t.
Yes: there can be functions in our code or in libraries that we call
that return other values than 1 to mean "true", although we tend to
avoid doing so in our own code.
> And what if TRUE happens to be defined as -1?
It never will be. It's defined as 1 specifically so that it matches the
C language definition of "true".
> (In that case unknown and FALSE would have the same value)
- Julian
RE: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
Bert Huijben wrote:
> > -----Original Message-----
> > From: Stefan Fuhrmann [mailto:stefanfuhrmann@alice-dsl.de]
> > 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();
>
> What do you try to get as result here?
>
> I'm not 100% sure if tests like (12 == 12) really always return the defined
> value TRUE
(I'm just addressing the C language facts in this response.)
In C, when an operator such as "==" is defined to yield a Boolean value,
it *always* return 0 for "false" or 1 for "true", never other values for
"true".
However, non-Boolean-valued expressions, such as "flags & 0x40", can be
used where a Boolean value is wanted, such as "if (flags & 0x40)" or
"override || (flags & 0x40)". Any non-zero is regarded as "true".
> and we most likely have other functions that return just some !=
> 0 value as svn_boolean_t.
Yes: there can be functions in our code or in libraries that we call
that return other values than 1 to mean "true", although we tend to
avoid doing so in our own code.
> And what if TRUE happens to be defined as -1?
It never will be. It's defined as 1 specifically so that it matches the
C language definition of "true".
> (In that case unknown and FALSE would have the same value)
- Julian
RE: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Fuhrmann [mailto:stefanfuhrmann@alice-dsl.de]
> Sent: donderdag 4 november 2010 0:21
> To: Subversion Development
> Subject: [Patch] Make svn_tristate_t compatible with svn_boolean_t
>
> 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();
What do you try to get as result here?
I'm not 100% sure if tests like (12 == 12) really always return the defined
value TRUE and we most likely have other functions that return just some !=
0 value as svn_boolean_t.
And what if TRUE happens to be defined as -1?
(In that case unknown and FALSE would have the same value)
I don't think this change makes it easier to use the trystate. You still
have to perform the checks yourself.
Bert
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-11-10, Greg Stein wrote:
> It is used *once* ?!
>
> Bleh. Then I'd ask why we even have the type, especially why does it
> exist in svn_types.h.
I wondered the same. The answer appears to be that
'svn_log_changed_path2_t' (which was added to svn_types.h in 1.6) has
now been extended with two new fields, 'text_modified' and
'props_modified', which are only valid in certain cases, as part of
r877688:
> r877688 | rhuijben | 2009-05-06 17:02:16 +0100 (Wed, 06 May 2009) | 46 lines
>
> Provide information on whether the text and/or properties were changed for
> each changed path in a revision. This patch makes the information available
> to api users and the svn log -v --xml output.
>
> * subversion/include/svn_types.h
> (svn_tristate_t): New type.
> (svn_tristate_to_word, svn_tristate_from_word): New function.
> (svn_log_changed_path2_t): Add two new fields.
>
> * subversion/libsvn_ra_neon/log.c
> (log_start_element): Parse text-mods and props-mods attributes.
>
> * subversion/libsvn_ra_serf/log.c
> (read_changed_path_attributes): New function to parse node kind and
> modification types.
> (start_log): Move node kind parsing to read_changed_path_attributes.
>
> * subversion/libsvn_ra_svn/client.c
> (optbool_to_tristate): New function.
> (ra_svn_log): Add parsing of two new optional booleans.
>
> * subversion/libsvn_ra_svn/protocol
> (changed-path-entry): Fix optional parts for node kind and add text-mods
> and prop-mods.
>
> * subversion/libsvn_repos/log.c
> (detect_changed): Copy text and property modified values from fs api.
>
> * subversion/libsvn_subr/kitchensink.c
> (svn_tristate_to_word, svn_tristate_from_word): New function.
>
> * subversion/mod_dav_svn/reports/log.c
> (log_receiver): Move common attribute generation code to a single place and
> provide text-mods and prop-mods attributes.
>
> * subversion/svn/log-cmd.c
> (log_entry_receiver_xml): Add text-mods and prop-mods attributes.
>
> * subversion/svn/schema/log.rnc
> (attlist.path): Declare text-mods and prop-mods attributes.
>
> * subversion/svnserve/serve.c
> (log_receiver): Provide two extra booleans, removing optional definition
> from the node-kind as we always provide this. (Writing optional booleans
> is not supported by the svn tuple code).
The question is: would we prefer to represent the data another way? For
example, we could perhaps make them booleans and say that if both are
false when action = 'M', that means the specific data is NOT available,
and if either or both is true then that means it IS available.
Stefan2 has gone on to use the tristate type for two more purposes in
his performance branch, but I'm not sure that we've yet demonstrated
it's worth making it a (svn-)global public type.
- Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Greg Stein <gs...@gmail.com>.
It is used *once* ?!
Bleh. Then I'd ask why we even have the type, especially why does it
exist in svn_types.h.
Your other points: quite fair. I assumed tristate_t was much more
important due to its placement in svn_types.
Cheers,
-g
On Tue, Nov 9, 2010 at 12:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> I would +1 everything you said... if Julian had changed svn_boolean_t.
>
> He changed svn_tristate_t, which is used exactly once in trunk, so
> I don't see the need to be as careful with it.
>
> The caveat? This assumes that --- as we ought to --- we not act on
> feedback differently because it comes after the change has been
> committed. Feedback that would have caused a "No, let's not commit this
> yet" reaction if provided early should cause a "OK, let's revert this
> for now then" reaction if provided later.
>
> Daniel
>
> Greg Stein wrote on Tue, Nov 09, 2010 at 00:47:27 -0500:
>> On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright
>> <hy...@mail.utexas.edu> wrote:
>> > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@wandisco.com> wrote:
>> >...
>> >> 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?
>>
>> I have raised this before, Julian. For making changes with some
>> impact, where feedback from the community is desired, then the
>> standard Apache rule is 72 hours. And even if we don't worry about
>> rules, it is simply *respectful* to give others a chance to speak up.
>>
>> Last time, you bumped the format with something like FOUR hours
>> notice. That was really bad. I'm not sure about the extent of the
>> badness here (tho changing something as central as svn_types.h seems
>> pretty obvious as an "input-required" change), but given that another
>> member of the community has said "woah. too quick", then you REALLY
>> need to slow down.
>>
>> This isn't an attempt to slow down your work. This is an attempt
>> (bordering on requirement) for you to work with the rest of the
>> community on changes to our codebase.
>>
>> -g
>
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I would +1 everything you said... if Julian had changed svn_boolean_t.
He changed svn_tristate_t, which is used exactly once in trunk, so
I don't see the need to be as careful with it.
The caveat? This assumes that --- as we ought to --- we not act on
feedback differently because it comes after the change has been
committed. Feedback that would have caused a "No, let's not commit this
yet" reaction if provided early should cause a "OK, let's revert this
for now then" reaction if provided later.
Daniel
Greg Stein wrote on Tue, Nov 09, 2010 at 00:47:27 -0500:
> On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
> > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@wandisco.com> wrote:
> >...
> >> 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?
>
> I have raised this before, Julian. For making changes with some
> impact, where feedback from the community is desired, then the
> standard Apache rule is 72 hours. And even if we don't worry about
> rules, it is simply *respectful* to give others a chance to speak up.
>
> Last time, you bumped the format with something like FOUR hours
> notice. That was really bad. I'm not sure about the extent of the
> badness here (tho changing something as central as svn_types.h seems
> pretty obvious as an "input-required" change), but given that another
> member of the community has said "woah. too quick", then you REALLY
> need to slow down.
>
> This isn't an attempt to slow down your work. This is an attempt
> (bordering on requirement) for you to work with the rest of the
> community on changes to our codebase.
>
> -g
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I would +1 everything you said... if Julian had changed svn_boolean_t.
He changed svn_tristate_t, which is used exactly once in trunk, so
I don't see the need to be as careful with it.
The caveat? This assumes that --- as we ought to --- we not act on
feedback differently because it comes after the change has been
committed. Feedback that would have caused a "No, let's not commit this
yet" reaction if provided early should cause a "OK, let's revert this
for now then" reaction if provided later.
Daniel
Greg Stein wrote on Tue, Nov 09, 2010 at 00:47:27 -0500:
> On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright
> <hy...@mail.utexas.edu> wrote:
> > On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@wandisco.com> wrote:
> >...
> >> 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?
>
> I have raised this before, Julian. For making changes with some
> impact, where feedback from the community is desired, then the
> standard Apache rule is 72 hours. And even if we don't worry about
> rules, it is simply *respectful* to give others a chance to speak up.
>
> Last time, you bumped the format with something like FOUR hours
> notice. That was really bad. I'm not sure about the extent of the
> badness here (tho changing something as central as svn_types.h seems
> pretty obvious as an "input-required" change), but given that another
> member of the community has said "woah. too quick", then you REALLY
> need to slow down.
>
> This isn't an attempt to slow down your work. This is an attempt
> (bordering on requirement) for you to work with the rest of the
> community on changes to our codebase.
>
> -g
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Nov 4, 2010 at 08:45, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
> On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@wandisco.com> wrote:
>...
>> 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?
I have raised this before, Julian. For making changes with some
impact, where feedback from the community is desired, then the
standard Apache rule is 72 hours. And even if we don't worry about
rules, it is simply *respectful* to give others a chance to speak up.
Last time, you bumped the format with something like FOUR hours
notice. That was really bad. I'm not sure about the extent of the
badness here (tho changing something as central as svn_types.h seems
pretty obvious as an "input-required" change), but given that another
member of the community has said "woah. too quick", then you REALLY
need to slow down.
This isn't an attempt to slow down your work. This is an attempt
(bordering on requirement) for you to work with the rest of the
community on changes to our codebase.
-g
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-11-04 at 07:45 -0500, Hyrum K. Wright wrote:
> On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@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
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Thu, Nov 4, 2010 at 7:43 AM, Julian Foad <ju...@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)
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2010-11-07, Stefan Fuhrmann wrote:
[...]
> Thanks all for the replies. Here is how I see it plus
> a couple of things I discovered in the meantime.
>
> * general consensus: overlapping definitions with
> inconsistent meaning are bad.
> * enums are always compatible with ints, so we can't
> make them incompatible at compile time
We can't make *enums* incompatible, but we could make the tristate a
different type, such as a struct or a pointer, which would be
incompatible. That can be option (3). But it doesn't seem to be
necessary, as your option (2) seems to provide good enough separation.
> That leaves us with two basic options.
>
> (1) Define that a tristate is a "nullable" boolean (.net speak).
> To me, this seems to be the way that it is being used
> right now. However, one might be tempted to (miss-)use
> tristates instead of booleans "just in case". That would
> be bad.
>
> (2) Define that tristates and booleans are similar but
> fundamentally different. Using values >1 for tristate
> values would make them "always true" in boolean
> expressions. So, the logic would probably fail early
> and the compiler might even generate a warning.
>
> I think there is no big risk associated with neither
> of these options. If people feel uncomfortable with (1),
> I'm happy to switch to (2).
That would be OK for me.
> But I also discovered two issues with the current boolean
> type definitions. First, TRUE and FALSE are defined
> after I use them in svn_tristate_t which can be fixed easily
> and leads directly to the second issue.
>
> We only define TRUE and FALSE in case they have
> not been defined, yet. Since we rely on their numerical
> values, we should at test for these values in case the
> macros are pre-defined (#error if not).
That's over-kill. Not necessary.
> If people agree, I will implement (2) and the fix the other
> two issues.
- Julian
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 04.11.2010 13:47, Hyrum K. Wright wrote:
> On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad<ju...@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.
>
Thanks all for the replies. Here is how I see it plus
a couple of things I discovered in the meantime.
* general consensus: overlapping definitions with
inconsistent meaning are bad.
* enums are always compatible with ints, so we can't
make them incompatible at compile time
That leaves us with two basic options.
(1) Define that a tristate is a "nullable" boolean (.net speak).
To me, this seems to be the way that it is being used
right now. However, one might be tempted to (miss-)use
tristates instead of booleans "just in case". That would
be bad.
(2) Define that tristates and booleans are similar but
fundamentally different. Using values >1 for tristate
values would make them "always true" in boolean
expressions. So, the logic would probably fail early
and the compiler might even generate a warning.
I think there is no big risk associated with neither
of these options. If people feel uncomfortable with (1),
I'm happy to switch to (2).
But I also discovered two issues with the current boolean
type definitions. First, TRUE and FALSE are defined
after I use them in svn_tristate_t which can be fixed easily
and leads directly to the second issue.
We only define TRUE and FALSE in case they have
not been defined, yet. Since we rely on their numerical
values, we should at test for these values in case the
macros are pre-defined (#error if not).
If people agree, I will implement (2) and the fix the other
two issues.
-- Stefan^2.
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Thu, Nov 4, 2010 at 6:51 AM, Julian Foad <ju...@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
Re: [Patch] Make svn_tristate_t compatible with svn_boolean_t
Posted by Julian Foad <ju...@wandisco.com>.
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. :-/
- Julian