jerryscript icon indicating copy to clipboard operation
jerryscript copied to clipboard

Refactoring ecma_extended_object_t::cls:: to ease the coding about builtin object class

Open lygstate opened this issue 11 months ago • 8 comments
trafficstars

Currently all builtin class are sharing same union u1 u2 u3 in ecma_extended_object_t::cls::, that's complicated the things

Now we split general object class into ecma_object_cls_general_t and can only access with ecma_object_cls_general, for others split it into separate struct, so when new builtin object class, bug-fixing, feature improving will be easier.

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

lygstate avatar Nov 25 '24 09:11 lygstate

The point of the single union is that it is not possible to use multiple variables of a union for data storage (and overwriting each other). While this patch improves readability, people may accidentally think that arraybuffer / typedarray are different things and may use both. Anyway I am neutral for this change, but wanted to add some notes about the original purpose.

zherczeg avatar Nov 25 '24 09:11 zherczeg

Btw, am I see correctly that it increases the structure total size?

zherczeg avatar Nov 25 '24 09:11 zherczeg

Btw, am I see correctly that it increases the structure total size?

OK, let's me use static_assert to ensure that, the total size is not changed

lygstate avatar Nov 25 '24 10:11 lygstate

The point of the single union is that it is not possible to use multiple variables of a union for data storage (and overwriting each other). While this patch improves readability, people may accidentally think that arraybuffer / typedarray are different things and So a note in comment to tell that fact they can not be used both?

may use both. Anyway I am neutral for this change, but wanted to add some notes about the original purpose. There is a advange of doing this, that is we can extend the arraybuffer dataview to support uint52_t

lygstate avatar Nov 25 '24 10:11 lygstate

OK, let's me use static_assert to ensure that, the total size is not changed

As far as I see now we have a cls and clz unions, and they are independent members of the parent structure. This is why I worry about the structure size.

zherczeg avatar Nov 25 '24 10:11 zherczeg

OK, let's me use static_assert to ensure that, the total size is not changed

As far as I see now we have a cls and clz unions, and they are independent members of the parent structure. This is why I worry about the structure size.

Is this the right direction, I can convert all the reamaing to separate class, then I can merge cls and clz

lygstate avatar Nov 25 '24 10:11 lygstate

OK, let's me use static_assert to ensure that, the total size is not changed

As far as I see now we have a cls and clz unions, and they are independent members of the parent structure. This is why I worry about the structure size.

Your concern addressed, ecma_object_cls_general are used for access general(boolean,string,number,bigint,symbol) object, and other special object class use related struct like cls::typedarray and so on, now they can be accessed in clear way

ecma_object_cls_general have ASSERT to ensure there is no mis-access

lygstate avatar Nov 25 '24 22:11 lygstate

@zherczeg all your concern addressed, please take a look again

lygstate avatar Nov 26 '24 13:11 lygstate