add MMD bug in Integer PMC

[Andrew Whitworth]

This is the offending code: from src/pmc/integer.pmc:

MULTI PMC *add(DEFAULT value, PMC *dest) {
        dest = pmc_new(INTERP, VTABLE_type(interp, value));

        VTABLE_set_number_native(INTERP, dest,
                SELF.get_integer() + VTABLE_get_number(INTERP, value));
        return dest;
    }

I suggest that the middle line there be changed to:

INTVAL myvalue = SELF.get_integer();
INTVAL othervalue = VTABLE_get_integer(INTERP, value);
VTABLE_set_integer_native(INTERP, dest, myvalue + othervalue);

Has these benefits:

1) The result type is whatever type the second value is, so it
respects subclassing rules for HLLs
2) Subclasses of Integer, or integral types need only define the
get_integer and set_integer_native VTABLEs to interact with an
Integer, as we should expect
3) Integer gets to act on INTVALs in a consistent way.

I do not see any advantage to having the Integer.add VTABLE use the
set_number_native and get_number VTABLEs of the destination PMC. If
another PMC type wants to be able to interact with Integer, it should
define the set_integer_native and get_integer VTABLEs.


--Andrew Whitworth



On Fri, Oct 30, 2009 at 12:14 PM, Jonathan Leto <jaleto at gmail.com> wrote:
> Howdy,
>
> I think this is wrong and has been a long-standing bug in our code:
>
> The Integer PMC, in the default case of it's add MMD, treats values as
> Floats and returns a Float
>
> I would like to treat it as such and write a failing test for it, fix
> the bug, and then change the behavior. Is there any reason not to do
> this?