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.

Sign up for GitHub

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

Jump to bottom

Outsource magic error messages #4821

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Outsource magic error messages #4821

merged 1 commit into from
Nov 25, 2021

Conversation

repasics
Copy link
Contributor

Modify tools/gen-magic-strings.py to generate error messages.

JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected]

@repasics repasics force-pushed the outsource_magic_error_msgs branch 2 times, most recently from a0c7c39 to 1085deb Compare November 10, 2021 15:37
galpeter
galpeter reviewed Nov 10, 2021
View reviewed changes
jerry-core/ecma/base/ecma-errors.h Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-errors.c Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-errors.h Show resolved Hide resolved
zherczeg
zherczeg requested changes Nov 10, 2021
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good patch. Are these strings available as magic strings? Since they are part of the binary anyway, there should be no memory allocation when these strings are used in JS code.

jerry-core/ecma/base/ecma-errors.c Outdated Show resolved Hide resolved
@zherczeg
Copy link
Member

After #4816, parser errors should be externalized as well.

@repasics repasics force-pushed the outsource_magic_error_msgs branch 3 times, most recently from 85e6ee5 to c821851 Compare November 11, 2021 14:24
@repasics
Copy link
Contributor Author

PR was refreshed: Generate error messages with magic strings using the same ini file.

@repasics repasics force-pushed the outsource_magic_error_msgs branch from c821851 to 21ddd03 Compare November 11, 2021 15:08
rerobika
rerobika requested changes Nov 11, 2021
View reviewed changes
tests/unit-core/test-to-length.c Outdated Show resolved Hide resolved
tests/unit-core/test-to-integer.c Outdated Show resolved Hide resolved
@repasics repasics force-pushed the outsource_magic_error_msgs branch 2 times, most recently from dd02d98 to 749773a Compare November 22, 2021 09:34
rerobika
rerobika requested changes Nov 22, 2021
View reviewed changes
jerry-core/ecma/base/ecma-errors.c Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-errors.c Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-errors.c Show resolved Hide resolved
@repasics repasics force-pushed the outsource_magic_error_msgs branch from 749773a to 949b21a Compare November 22, 2021 12:02
@repasics repasics force-pushed the outsource_magic_error_msgs branch from 949b21a to 82bbba3 Compare November 23, 2021 09:28
@repasics
Copy link
Contributor Author

The PR is updated.

@repasics repasics force-pushed the outsource_magic_error_msgs branch from 82bbba3 to dafd543 Compare November 23, 2021 11:26
galpeter
galpeter requested changes Nov 23, 2021
View reviewed changes
jerry-core/ecma/base/ecma-errors.h Outdated
#include "ecma-error-messages.inc.h"
#undef ECMA_ERROR_DEF
/** @endcond */
ECMA_IS_VALID_CONSTRUCTOR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok tho have this here? I don't see a relevant entry in the .c file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in ecma-function-object.c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, could we add a comment here why is this needed? (maybe where it is used?)

jerry-core/ecma/base/ecma-errors.h Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-globals.h Outdated Show resolved Hide resolved
@repasics repasics force-pushed the outsource_magic_error_msgs branch from dafd543 to 0cb587d Compare November 23, 2021 11:58
dbatyai
dbatyai reviewed Nov 23, 2021
View reviewed changes
jerry-core/ecma/base/ecma-errors.h Outdated Show resolved Hide resolved
jerry-core/ecma/operations/ecma-exceptions.c Outdated Show resolved Hide resolved
@repasics repasics force-pushed the outsource_magic_error_msgs branch 3 times, most recently from 56bf3e3 to a8c3af5 Compare November 23, 2021 12:43
@rerobika rerobika mentioned this pull request Nov 10, 2021
Merged
galpeter
galpeter reviewed Nov 24, 2021
View reviewed changes
jerry-core/ecma/operations/ecma-function-object.c Outdated Show resolved Hide resolved
jerry-core/ecma/operations/ecma-function-object.c Outdated Show resolved Hide resolved
jerry-core/ecma/base/ecma-errors.h Outdated
#include "ecma-error-messages.inc.h"
#undef ECMA_ERROR_DEF
/** @endcond */
ECMA_IS_VALID_CONSTRUCTOR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, could we add a comment here why is this needed? (maybe where it is used?)

tools/gen-strings.py Outdated Show resolved Hide resolved
@repasics repasics force-pushed the outsource_magic_error_msgs branch 3 times, most recently from 12b2a29 to 2d931a4 Compare November 25, 2021 12:02
galpeter
galpeter approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@galpeter galpeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change request, otherwise lgtm

jerry-core/ecma/base/ecma-errors.c Outdated
Comment on lines 53 to 54
#endif /* JERRY_ERROR_MESSAGES */

/**
* Error message, Cannot allocate memory for literals
*/
const char* const ecma_error_cannot_allocate_memory_literals = "Cannot allocate memory for literals";
} /* ecma_get_error_utf8 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this newline always here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it.

@repasics
Outsource magic error messages
e375f03 
Modify tools/gen-magic-strings.py to generate error messages.

JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected]
@repasics repasics force-pushed the outsource_magic_error_msgs branch from 2d931a4 to e375f03 Compare November 25, 2021 12:26
zherczeg
zherczeg approved these changes Nov 25, 2021
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rerobika rerobika merged commit 271d9b2 into jerryscript-project:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rerobika rerobika rerobika approved these changes

@galpeter galpeter galpeter approved these changes

@zherczeg zherczeg zherczeg approved these changes

@dbatyai dbatyai Awaiting requested review from dbatyai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@repasics @zherczeg @dbatyai @galpeter @rerobika