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

parser: rework error handling #178

Closed
wants to merge 2 commits into from
Closed

Conversation

belochub
Copy link
Member

Avoid using TryCatch introduced in
29b3e19 by using MaybeLocal instead.

Refs: #143

Avoid using TryCatch introduced in
29b3e19 by using
MaybeLocal instead.

Refs: #143
@belochub belochub requested a review from aqrln May 24, 2017 18:07
@@ -273,7 +269,7 @@ Local<Value> ParseBool(Isolate* isolate,
*size = 5;
} else {
THROW_EXCEPTION(TypeError, "Invalid format: expected boolean");
result = Undefined(isolate);
result = MaybeLocal<Value>();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the very same value that result was initialized with at line 263?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, right, I will remove redundant assignments, thanks for catching that 😃

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM.

return Undefined(isolate);
}
MaybeLocal<Value> result;
result = (kParseFunctions[type])(isolate, str + start_pos, end, &parsed_size);
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit but maybe it's better to make it into one statement? =)

return Undefined(isolate);
}
MaybeLocal<Value> t;
t = kParseFunctions[current_type](isolate,
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, this one will event fit actually.

Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Awesome, thanks. Not only this easier to read and maintain, but it also makes parsing up to 10% faster, from what I see.

@aqrln
Copy link
Member

aqrln commented May 26, 2017

Landed in a67612528d832ea9349e36093a6d3a941caaae22 6909fab. Thanks!

@aqrln aqrln closed this May 26, 2017
aqrln pushed a commit that referenced this pull request May 26, 2017
Avoid using TryCatch introduced in
29b3e19 by using
MaybeLocal instead.

PR-URL: #178
Refs: #143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@aqrln aqrln deleted the rework-parser-errors branch May 26, 2017 16:54
belochub added a commit that referenced this pull request Jan 22, 2018
Avoid using TryCatch introduced in
29b3e19 by using
MaybeLocal instead.

PR-URL: #178
Refs: #143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
belochub added a commit that referenced this pull request Jan 22, 2018
Avoid using TryCatch introduced in
29b3e19 by using
MaybeLocal instead.

PR-URL: #178
Refs: #143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@belochub belochub mentioned this pull request Jan 22, 2018
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
Avoid using TryCatch introduced in
29b3e197ae4c33d1b9057b57731b4c422de764f3 by using
MaybeLocal instead.

PR-URL: metarhia/jstp#178
Refs: metarhia/jstp#143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
Avoid using TryCatch introduced in
29b3e197ae4c33d1b9057b57731b4c422de764f3 by using
MaybeLocal instead.

PR-URL: metarhia/jstp#178
Refs: metarhia/jstp#143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 21, 2018
Avoid using TryCatch introduced in
29b3e197ae4c33d1b9057b57731b4c422de764f3 by using
MaybeLocal instead.

PR-URL: metarhia/jstp#178
Refs: metarhia/jstp#143
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants