rational.pmc's get_bool is backwards and not portable

[doughera]

I am seeing the following failure in t/dynpmc/rational.t:

./parrot t/dynpmc/rational.t 
1..78
Floating point exception

The problem is that the Rational get_bool vtable does this:

    if (mpq_cmp_si(RT(SELF), 0, 0))
        return 0;
    else 
        return 1;

That is, it compares a value with 0/0, which, naturally, raises a divide-by-zero exception. (I haven't traced it carefully, but I think if gmp is compiled by gcc, a different set of macros kick in, which handle this case differently.) The sense of the comparison is strange too: Why is 0/0 considered "true", but everything else "false"? I think it's backwards by accident.

The failure is first triggered by the test_init function, which does

.sub test_init
    new $P1, 'Rational'
    ok($P1,'initialization')
.end

The ok function is the one that ends up calling get_bool.

The basic question is this: Why not compare with 0/1? This is at least reasonably analagous to what the get_bool vtable does for integers, and avoids any corner cases with 0/0.

The attached patch does that. It also fixes two tests: First, test_init was backwards. The initial default value is 0/1 (which was previously testing as "ok", since the cmp test was backwards). The test now explicitly tests for the expected default value. Second, test_bool now works, and tests both true and false.

[plobsing]

Replying to doughera:

The basic question is this: Why not compare with 0/1? This is at least reasonably analagous to what the get_bool vtable does for integers, and avoids any corner cases with 0/0.

A more fundamental question is: why do we want a get_bool() vtable on rationals? simply because we can?

get_bool() on integer only makes sense because of the pervasive legacy of C and its idioms. Extending this to rationals doesn't make a lot of sense to me.

[whiteknight]

I agree with plobsing here. There's no real reason to treat a rational as a boolean. I don't think asking a Rational for it's status as a boolean really carries any meaning, and I don't think users are going to have any particular expectation for it.

I say we rip that VTABLE out, and replace the test to verify that the rational is not null or something else.

[doughera]

Well, I don't think it's that much of a stretch to define a boolean operator in parrot for this, and the author of the test_init sub quoted above apparently thought it a natural thing to use as well. Still, I have no strong opinion either way.

[cotto]

The real problem to me is that Rational isn't living somewhere where people care about it. Rational.get_bool indeed seems backwards and broken and should be fixed, but I think we should also plan on moving it into a separate library of math-related PMCs and ops at some point.

[pmichaud]

Replying to plobsing:

Replying to doughera:

The basic question is this: Why not compare with 0/1? This is at least reasonably analagous to what the get_bool vtable does for integers, and avoids any corner cases with 0/0.

A more fundamental question is: why do we want a get_bool() vtable on rationals? simply because we can? get_bool() on integer only makes sense because of the pervasive legacy of C and its idioms. Extending this to rationals doesn't make a lot of sense to me.

As a counter-example, Perl 6 has rationals as one of its fundamental data types (the results of most division operations is a rational), and it *definitely* makes sense to test rationals for boolean there. Parrot can of course say "it doesn't make any sense", but Perl 6 believes it does (and all core Perl 6 programmers would tend to agree).

This isn't to say that Rakudo is likely to use the Rational PMC (it isn't); it's just pointing out that there is at least one language that thinks testing rationals for boolean makes sense.

Pm

[dukeleto]

I think it is very intuitive to ask a Rational for a boolean value. We should keep and fix this behaviour.

[doughera]

This test still fails in 3.0.0. While debating whether or not to remove get_bool, or even rational.pmc entirely, it would probably be useful to apply the attached patch so that at least the test no longer failed.

[dukeleto]

Testing this patch on my machine now.

[dukeleto]

Thanks! Patch applied in 09258d5 and a passing smoke on my machine:

 http://smolder.parrot.org/app/projects/report_details/3971