Ticket #1965 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

rational.pmc's get_bool is backwards and not portable

Reported by: doughera Owned by: dukeleto
Priority: normal Milestone:
Component: testing Version: 2.11.0
Severity: medium Keywords:
Cc: Language:
Patch status: applied Platform:

Description

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.

Attachments

tt1965.patch Download (1.2 KB) - added by doughera 4 years ago.

Change History

Changed 4 years ago by doughera

in reply to: ↑ description ; follow-up: ↓ 5   Changed 4 years ago by 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.

  Changed 4 years ago by 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.

  Changed 4 years ago by 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.

  Changed 4 years ago by 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.

in reply to: ↑ 1   Changed 4 years ago by 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

  Changed 4 years ago by dukeleto

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

  Changed 4 years ago by jkeenan

  • component changed from none to testing

  Changed 4 years ago by 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.

  Changed 4 years ago by dukeleto

  • owner set to dukeleto
  • status changed from new to assigned

Testing this patch on my machine now.

  Changed 4 years ago by dukeleto

  • status changed from assigned to closed
  • resolution set to fixed
  • patch changed from new to applied

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

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

Note: See TracTickets for help on using tickets.