Backport #6044

Float#% bug in cornercases

Added by Marc-Andre Lafortune over 3 years ago. Updated about 3 years ago.

[ruby-core:42726]
Status:Closed
Priority:Normal
Assignee:Yui NARUSE

Description

On my platform, current behavior is:

4.0 % Float::INFINITY # => NaN
4.0.send :%, Float::INFINITY # => 4.0

-0.0 % 42 # => 0.0
-0.0.send(:%, 42) # => -0.0

On some platforms, these might return NaN and 0.0 in all cases.

My proposed behavior is to return 4.0 and -0.0 on all cases and on all platforms.

I'm tempted to assume it is clear that this is bug and that my proposed behavior is the right solution, but let me use my guidelines:

Proposed behavior passes my "strict superiority test" as it is clearly more consistent:
* consistent for different calling methods
* consistent accross platforms
* consistent with 4 % Float::INFINITY
* consistent with the IEEE definition of fmod (see http://pubs.opengroup.org/onlinepubs/007904975/functions/fmod.html )

It is also more intuitive and useful, since:

any_small_number % any_big_number == any_small_number

Current behavior passes the "clear defect test" as it is platform dependent (when it can reasonable be platfom independent). I'll add to the list of "clear defect" criteria the fact that calling an operator directly doesn't yield the same result as using #send.

The proposed solution fails my "straightforward" test as the proposed behavior contradicts part of the documentation which states "x.modulo(y) means x-y*(x/y).floor".

Any objection for me to commit this?

Thanks

Marc-André

fixmodulo.patch Magnifier (2.7 KB) Marc-Andre Lafortune, 02/18/2012 09:26 AM

Associated revisions

Revision 35013
Added by Marc-Andre Lafortune about 3 years ago

  • numeric.c: fix flodivmod for cornercases [Bug #6044]
    add ruby_float_mod

  • insns.def (opt_mod): use ruby_float_mod

  • internal.h: declare ruby_float_mod

  • test/ruby/test_float.rb: tests for above

  • test/ruby/envutil.rb: create helper assert_is_minus_zero

Revision 35176
Added by Yui NARUSE about 3 years ago

merge revision(s) 35013:

* numeric.c: fix flodivmod for cornercases [Bug #6044]
  add ruby_float_mod

* insns.def (opt_mod): use ruby_float_mod

* internal.h: declare ruby_float_mod

* test/ruby/test_float.rb: tests for above

* test/ruby/envutil.rb: create helper assert_is_minus_zero

History

#1 Updated by Yui NARUSE over 3 years ago

I agree the point.

About the implementation, flodivmod() should be public but internal API and declare prototype in internal.h,
and use it from insns.def.

After commit this, please transfer this ticket to Backport93.

#2 Updated by Marc-Andre Lafortune about 3 years ago

Additional problem case:

4.2 % 0 # => ZeroDivisionError
4.2 % 0.0 # => NaN
4.2.send :%, 0.0 # => ZeroDivisionError

Because returning NaN is not a useful result and to be consistent with BigDecimal, all three will raise a ZeroDivisionError unless someone has arguments to the contrary.

Yui NARUSE wrote:

About the implementation, flodivmod() should be public but internal API and declare prototype in internal.h,
and use it from insns.def.

Ok, thanks, I will do that.

#3 Updated by Marc-Andre Lafortune about 3 years ago

  • Tracker changed from Bug to Backport
  • Project changed from Ruby trunk to Backport193
  • Category deleted (core)
  • Target version deleted (2.0.0)

Done in r35013.

Moving to backport

#4 Updated by Marc-Andre Lafortune about 3 years ago

  • Assignee changed from Marc-Andre Lafortune to Yui NARUSE

#5 Updated by Yui NARUSE about 3 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r35176.
Marc-Andre, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


merge revision(s) 35013:

* numeric.c: fix flodivmod for cornercases [Bug #6044]
  add ruby_float_mod

* insns.def (opt_mod): use ruby_float_mod

* internal.h: declare ruby_float_mod

* test/ruby/test_float.rb: tests for above

* test/ruby/envutil.rb: create helper assert_is_minus_zero

Also available in: Atom PDF