0x7CFE / llst

LLVM powered Little Smalltalk.
Other
93 stars 10 forks source link

Steal class TInteger from burdakovd #51

Closed kpp closed 10 years ago

kpp commented 10 years ago

Daniil had written a good class TInteger in burdakovd/llst@877d6ece7742d621494f83cc1a7865d20f604219. This class allows us to reduce the number of usage functions getIntegerValue and newInteger. Also we may rename the class TInteger to TSmallInt.

burdakovd commented 10 years ago

Sorry guys. Was too swamped (just left my current job, going to relocate to another country, etcetera etcetera) to tidy up the code properly and do a pull request.

Just want to point out a few things (which I don't see an easy way to fix) to consider before adopting this class:

// The illustration for the last issue
// If the instance was created from smalltalk, then field1 contains null.
// And interpreting this null as TInteger is illegal.
struct MyClass : public TObject {
    TInteger field1;

    // native method
    void inc() { field1 = static_cast<int32_t>(feild1) + 1; }
};
kpp commented 10 years ago

It has implicit casts to int32t and to TObject, which makes the code ambiguous sometimes (e.g. TInteger x; int32t y = x + 5; // the compiler can't figure out if we want to convert TInteger to TObject or to int32_t). To fix this we can make one of the casts explicit - e.g. if we usually convert to int32_t, then replace operator TObject*() with a method called "asTObject()"

You are right. Moreover, operator T*() leads to a potencial error, which cannot be determined at compile time:

    TInteger x;
    delete x;

The class validates pointer in the constructor - it's safe and convenient, but might negatively impact performance where we often convert between pointers and integers and know that the pointers are correct integers (e.g. core of the VM)

I have checked this out. Performance is not affected.

I claimed that the class is binary compatible with "TObject_" - it's just my intuition, you'd better check with some C++ expert on this (e.g. there may be problems with alignment). Or at least add staticassert(sizeof(TObject) == sizeof(TInteger)); static_assert(sizeof(int32_t) == sizeof(TInteger));

No need. sizeof(pointer) == 4 bytes since we compile the code with -m32.

Since the validation is performed in the constructor it looks like every object of type TInteger always maintains the correct state. However, if we create a pointer inside the VM, and then reinterpret the data as TInteger - then the constructor will not be invoked and the validation will not be done.

It deserves further consideration and a special issue.