Too little MMD? i_add doesn't know about subclasses.

[coke]

partcl has a bug ( http://code.google.com/p/partcl/issues/detail?id=81) where TclInt+=a TclInt is now generating a TclFloat (instead of a TclInt.)

I suspect that this is a result of the change to src/pmc/integer.pmc which change i_add from a MULTI to a vtable-with-a-switch.

Originally:

-    MULTI void i_add(Integer value) {
-        STATICSELF.i_add_int(VTABLE_get_integer(INTERP, value));
-    }

This is now:

+    VTABLE void i_add(PMC *value) {
+        const INTVAL type = value->vtable->base_type;
+
+        switch (type) {
+            case enum_class_Integer:
+                STATICSELF.i_add_int(VTABLE_get_integer(INTERP, value));
+                break;

But since subclasses of Integer are not enum_class_Integer, subclasses seem to now fall through to:

+            default:
+                VTABLE_set_number_native(INTERP, SELF,
+                        SELF.get_integer() + VTABLE_get_number(INTERP, value));
+                break;

This is, from partcl's standpoint, a substantial feature change. This should probably be reverted, tests added, and if we want to go forward with this change, we can do it post 1.4.

Alternatively, we can update the switch statement to be functionally equivalent to the old multi. (and still add tests to verify that subclasses continue to work as expected.)

Apologies for not testing this sooner; partcl has been broken for other reasons, just got back to a point where this could be tested.

[coke]

Confirmed; If I revert r38000, then the partcl bug ( http://code.google.com/p/partcl/issues/detail?id=81) is resolved.

[Infinoid]

Replying to coke:

Confirmed; If I revert r38000, then the partcl bug ( http://code.google.com/p/partcl/issues/detail?id=81) is resolved.

Thanks. So apparently a switch statement around pmc->vtable->base_type isn't as smart as the previous MMD stuff was. After looking a little more closely, it seems the base_type field always contains the *current* type, not the base type. (For classes generated by pmc2c, at least.) I'm guessing MMD is smart enough to go look at the inheritance tree when it matches this up.

Could we replace the base_type stuff with a simple call to VTABLE_isa(), perhaps? I'd like to find a way to be smart about inherited types, and yet still retain some of the performance gains.

I'll attach a patch that does this.

Mark

[Infinoid]

Potential fix for Integer.i_add.

[coke]

The patch posted via nopaste (which looks like the one attached to the ticket) resolves the partcl bug also.

[fperrad]

Or you could add this code in src/pmc/tclint.pmc

    MULTI void i_add(TclInt value) {
        const INTVAL result = SELF.get_integer()
                            + VTABLE_get_integer(INTERP, value);
        SELF.set_integer_native(result);
    }

See PMC in WMLScript (or Lua) for a full example.

[coke]

If Infiniod's patch didn't work (it does), I would probably apply this as a stopgap, thanks.

In general, I'm like to avoid changing partcl to work around behavior changes in parrot that are occurring outside of the deprecation cycle; nor do I want to have to override all the various vtables when their behavior inadvertently changes. (This is just the one I happened to catch; See TT #452 for a bunch more that are coming.)

Note that this is not quite the same behavior, is it only fixes TclInts, not any other Integer subclasses I might happen to be thrown. (Which, in the absence of another HLL should be none.)

[whiteknight]

The big problem with this change that I can see is that Integer.i_add for some reason defaults to use set_number_native. That is not a sane fallback by any standards. Why should the result of an i_add on integer produce a floating point value?

[Infinoid]

Replying to whiteknight:

The big problem with this change that I can see is that Integer.i_add for some reason defaults to use set_number_native. That is not a sane fallback by any standards. Why should the result of an i_add on integer produce a floating point value?

I believe the intention is that if it doesn't recognize the second value as an integer type, it assumes float so that it won't lose any precision. With that default, "1 += 1.2" will result in 2.2, otherwise the result would be 2. That's my best guess, anyway, and I can't speak for whether the design is right here.

Also note, the TT #452 branch consolidates Integer's add and i_add functions, fixes the Complex case (which didn't make any attempt to preserve the imaginary portion), and automatically upgrades to BigInt when necessary. I generated this patch to test whether we understood the issue correctly, but in general, I'd prefer to coordinate with that branch rather than applying a specific fix for this (and causing merge conflicts).

Mark

[Infinoid]

Reverted r38000 in r39218. It sounds like the TT #452 goals are being reworked somewhat, to avoid breaking things like this in the future.