Misc #16750


Change typedef of VALUE for better type checking

Added by Dan0042 (Daniel DeLorme) over 1 year ago. Updated over 1 year ago.



VALUE is currently defined as typedef unsigned long VALUE, but as we all know that only creates an alias, not an actual type. Since the compiler gives no warnings when comparing with other integer values, it's easy to have bugs such as v == 42 which should have been v == INT2FIX(42). Actually not so long ago I saw nobu fixing a bug of that kind where an ID had been mixed up with a VALUE.

So in order to prevent these kinds of bugs I propose changing VALUE to a non-scalar type such as:

//example for 64-bit system
typedef union VALUE {
    struct RBasic* as_basic; //easy access to obj.as_basic->klass and obj.as_basic->flags
    void* as_ptr;            //can assign ptr = obj.as_ptr without a cast
    unsigned long i;         //raw int value for bitwise operations

    int immediate : 3;   //obj.immediate != 0 if obj is immediate type such as fixnum, flonum, static symbol

    int is_fixnum : 1;   //obj.is_fixnum == 1 if obj is fixnum

    struct {
        int flag : 2;    //obj.flonum.flag == 2 if obj is flonum
        int bits : 62;
    } flonum;

    struct {
        int flag : 8;    //obj.symbol.flag == 0x0c if obj is a static symbol
        int id : 56;     //-> == STATIC_SYM2ID(obj)
    } symbol;


This will allow proper type-checking via the compiler.

A little-known fact, structs and unions can be passed (and returned) by value to functions; this 64-bit union has the same performance as a 64-bit int. This approach also allows to simplify code like ((struct RBasic*)obj)->flags into the much more readable obj.as_basic->flags, and FIXNUM_P(obj) can be expressed directly as obj.is_fixnum. The only downside is that direct comparison of union variables is not possible, so you would need to use for example obj.i == Qfalse.i

To summarize, stricter type-checking would eliminate an entire class of bugs, and I estimate this change would require modifications to no more than 14% of the codebase (62,213 lines). Very much worth it I believe.


Also available in: Atom PDF