I just fixed a bug in the Integer PMC's i_multiply vtable ( TT #1887), but found some ugly looking code there that I think we should change. For i_multiply, we currently have this code:
VTABLE_set_number_native(INTERP, SELF, SELF.get_integer() * VTABLE_get_number(INTERP, value)); This is ugly for a number of reasons. The i_multiply_int vtable does overflow detection and throws an exception on overflow. This implementation here does not do any overflow detection, which breaks from the behavior of the other vtables. In addition, the Integer PMC is performing floating point arithmetic, which I don't think is consistent. Note that i_multiply is not the only VTABLE in Integer that performs floating-point arithmetic like this. What I would like to do instead is: VTABLE_i_multiply_int(INTERP, SELF, VTABLE_get_integer(INTERP, value)); However, this new implementation causes at least one test to break which relies on the current floating-point arithmetic behavior. As an alternative, though not nearly so friendly on the GC, we could do: PMC * const temp = VTABLE_multiply_int(INTERP, value, VTABLE_get_integer(INTERP, SELF)); VTABLE_i_multiply_int(INTERP, SELF, VTABLE_get_integer(INTERP, temp)); This second variation maintains the multiplication behavior of the more complicated type (all numeric types are more "complicated" than Integer), but still allows the Integer PMC to only explicitly perform integer arithmetic and not be casting to a FLOATVAL internally. Notice that if the integer PMC is performing all it's operations in floating-point, it really isn't any different from the FLOAT PMC except in the format in which intermediate results are stored. The Float PMC does have get_integer VTABLEs which will return an integer value. In most cases, the integer value returned from Float.get_integer will be the same value as the one returned by Integer.get_integer anyway. For the sake of consistency, I think we really have two options going forward: 1) We can change Integer to always perform integer arithmetic, and always do bounds-checking to detect overflow. 2) We can merge Integer and Float together into a single Number PMC, and rely on the get_integer and get_float VTABLEs to get the primitive values we want The former is my preferred solution. The later will require a deprecation cycle and significant developer effort, but we cut out a lot of duplicate code, simplify arithmetic handling, have consistent arithmetic behavior (but we lose optimizations for integer arithmetic, which we aren't benefiting from now either) etc. What I would like to do is redirect the i_multiply vtable to the i_multiply_int vtable, by calling VTABLE_get_integer on the incoming value. However, the current semantics use and expect the multiplication to be performed using floating-point arithmetic, not integer arithmetic. Input much appreciated. In either case we need a deprecation cycle (which I would like to be able to put in before 3.0 if possible), but there's win to be had if we can simplify some code and make some of our arithmetic more consistent. Thanks. --Andrew Whitworth _______________________________________________ http://lists.parrot.org/mailman/listinfo/parrot-dev
