Re: [boost] Re: Formal Review Tribool

From: Doug Gregor (gregod_at_[hidden])
Date: 2004-06-05 10:17:19

----- Original Message ----- 
From: "Guillaume Melquiond" <guillaume.melquiond_at_[hidden]>
> Operators ! && || have the expected semantic for tribool. It is not true
> for == and != (the values could also be directly compared) but I don't
> have a strong opinion on what the "good" semantic is.

Unfortunately, it depends on the application area. "if (x == true)" and "if
(x == false)" act the same way with both semantics (because of the bool
conversion), but "x == indeterminate" is always indeterminate in the current
semantics (which can be surprising)... I decided to go with the current
semantics because (1) it fit my application area and (2) it retains more
information.

> So the design is
> good, except for this point imho:
>
> Why is tribool default-initialized to false? Since tribool is supposed
> to mimick bool, I would rather have no default value. Or if a default
> value is necessary for practical use, indeterminate seems to me to be a
> better choice than false.

In retrospect, this was clearly a mistake. It should (and will) be
initialized to indeterminate.

> >      * What is your evaluation of the implementation?
>
> What is the purpose of safe_bool? I am asking the question because I was
> unable to compile with GCC a code as simple as:
>   tribool a; assert(a == a);
> GCC complains that it "cannot convert `boost::logic::tribool' to `long
> int' for argument `1' to `long int __builtin_expect(long int, long
> int)'". It is obviously a problem caused by the definition of assert in
> the library; but still... With a bool conversion operator, there is no
> such problem.

... and that should be safe for tribool. Sure, I'll make that a bool
conversion instead of a safe_bool.

> The definitions of the comparison operators are not optimal and the
> generated code is really bad. Something like this would be a lot more
> efficient (at least with GCC but there is no reason the other compilers
> would not benefit from this):
[snipcode]
> This code is 8 asm instructions long. The one in the tribool library
> (the commented line) produces more than 70 asm instructions. The same is
> true for !=.

Egads! Will fix.

> tribool_io.hpp refers to <boost/utility/noncopyable.hpp> but this header
> is not here.

Fixed locally.

> tribool_io_test.cpp should include <cassert>.

Will use the test lib (my local version already does).

> Concerning the tutorial, it should be mentioned the || and && operators
> are not short-circuiting operators contrarily to the boolean versions. I
> think it is important because it is a limitation of the implementation /
> language, not an inherent property of tribool.

Ah, good point.

> There is a problem with this example:
>   assert(!(y == y)); // okay, because y == y is indeterminate
> Since y == y is indeterminate, !(y == y) is too, and consequently assert
> should not be okay, should it?

Oops! Fixed.

> And just before, there is this comment:
>   // neither x nor y is false, but we don't know that both are true
> Shouldn't it be "neither x nor y is false and we know they are not both
> true"?

I think the original comment is better. Since one or both of the variables
is "indeterminate", we don't know if they are true. The comment is
addressing the truth of the  underlying properties of "x" and "y".

> The next comment also need to be modified.

How so? The "or both" was redundant, so I've eliminated it.

    Doug