-
Notifications
You must be signed in to change notification settings - Fork 684
Refactor literal-storage to not use C++ features #791
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
Conversation
Nice job! |
+1 |
46efb89
to
5219470
Compare
I've rebased this patch onto the latest master. Please, check https://github.com/Samsung/jerryscript/tree/refactor-lit-storage-rebased |
5219470
to
72c64df
Compare
Thank you Andrey! I've updated this PR. |
Measurement results: testing board: rpi2 master revision: 50d124b
|
* string */ | ||
record_t * | ||
create_charset_record (record_set_t *rec_set_p, /**< recordset */ | ||
const lit_utf8_byte_t *str, /**< string to be placed in the record */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- str -> str_p
- imho 'placed in the record' -> 'placed into the record'
extern bool rcs_record_is_equal (rcs_record_set_t *, rcs_record_t *, rcs_record_t *); | ||
extern bool rcs_record_is_equal_utf8 (rcs_record_set_t *, rcs_record_t *, const lit_utf8_byte_t *, lit_utf8_size_t); | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, we put comments after #endif
to describe condition, corresponding to code just above the #endif
. For example, !RCS_RECORDS_H
in the case.
Looks good to me |
c94f0a7
to
00379d6
Compare
#endif /* JERRY_ENABLE_SNAPSHOT */ | ||
extern rcs_record_t *lit_create_charset_literal (rcs_record_set_t *, const lit_utf8_byte_t *, lit_utf8_size_t); | ||
extern rcs_record_t *lit_create_magic_literal (rcs_record_set_t *, lit_magic_string_id_t); | ||
extern rcs_record_t *lit_create_magic_record_ex (rcs_record_set_t *, lit_magic_string_ex_id_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lit_create_magic_literal_ex
00379d6
to
f138345
Compare
uint32_t *); | ||
#endif /* JERRY_ENABLE_SNAPSHOT */ | ||
|
||
#endif /* RCS_SNAPSHOT_H */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be !RCS_SNAPSHOT_H
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should, I missed that.
f138345
to
9dec857
Compare
JERRY_ASSERT (RCS_RECORD_IS_MAGIC_STR_EX (rec_p)); | ||
|
||
rcs_record_set_field (rec_p, RCS_MAGIC_STR_HEADER_ID_POS, RCS_MAGIC_STR_HEADER_ID_WIDTH, id); | ||
} /* rcs_record_set_magic_str_id */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be: rcs_record_set_magic_str_ex_id
JerryScript-DCO-1.0-Signed-off-by: Roland Takacs [email protected]
9dec857
to
a997b59
Compare
lgtm |
merged: 21f561f |
@galpeter @sand1k and @ruben-ayrapetyan |
@jiangzidong Yes, that's correct. Several months ago a decision was made to move back to C and we are now incrementally rewriting the remaining C++ parts of JerryScript in C. |
@tilmannOSG Thanks. Do you have a schedule about when to complete the refactor |
@jiangzidong We don't have a strict schedule but we aim to complete the move to C in the coming months. |
@tilmannOSG I see, and I am working on removing c++ features of the rcs_chunked_list |
@jiangzidong That's great :) Contributions in that area are very welcome! |
This patch removes all C++ features from the literal-storage. The functionality is the same, just the code structure is different.
Measurement results: