Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lygstate
Copy link
Contributor

@lygstate lygstate commented Nov 25, 2024

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]

@zherczeg
Copy link
Member

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
Copy link
Member

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

@lygstate
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@zherczeg
Copy link
Member

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.

@lygstate
Copy link
Contributor Author

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 lygstate force-pushed the object_cls branch 3 times, most recently from c1ea60f to 7ee9f99 Compare November 25, 2024 21:55
@lygstate lygstate changed the title Refactoring ecma_extended_object_t::cls:: to ease the builtin Class coding Refactoring ecma_extended_object_t::cls:: to ease the coding about builtin object class Nov 25, 2024
@lygstate
Copy link
Contributor Author

lygstate commented Nov 25, 2024

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 lygstate force-pushed the object_cls branch 3 times, most recently from 98c3ee1 to 00db22d Compare November 26, 2024 05:35
…iltin object class

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants