Skip to content
New issue

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

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

Already on GitHub? # to your account

Refactor Error.stackTraceLimit #874

Merged
merged 3 commits into from
Feb 3, 2025
Merged

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Feb 1, 2025

  • Store the JS value, so the conversion happens when the backtrace is needed
  • Prevent recursion in build_backtrace
  • Simplify code for saving / restoring exception in build_backtrace

- Store the JS value, so the conversion happens when the backtrace is needed
- Prevent recursion in build_backtrace
- Simplify code for saving / restoring exception in build_backtrace
@saghul saghul requested a review from bnoordhuis February 1, 2025 23:20
@saghul
Copy link
Contributor Author

saghul commented Feb 1, 2025

@bnoordhuis PTAL, let me know what you think. It ended up a bit more complex that I originally envisioned.

Alternative: convert the value on the spot and store both the JS value and the converted value, and use the latter in build_backtrace.

Thoughts?

@saghul
Copy link
Contributor Author

saghul commented Feb 3, 2025

@bnoordhuis Updated, can you PTAL one more time?

JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit);
if (isnan(d) || d < 0.0 || (isinf(d) && d < 0.0))
stack_trace_limit = 0;
else if (isinf(d) || d > (double)INT32_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to cast to double (but it doesn't hurt either, it's just a no-op)

The isinf(d) && d < 0.0 two lines up looks superfluous. You check d < 0.0 first and -Infinity < 0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Does the same apply to checking for positive infinity? It will be larger than INT32_MAX I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Does the same apply to checking for positive infinity? It will be larger than INT32_MAX I guess.

Good point. By an infinite amount, according to mathematicians.

JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit);
if (isnan(d) || d < 0.0 || (isinf(d) && d < 0.0))
stack_trace_limit = 0;
else if (isinf(d) || d > (double)INT32_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Does the same apply to checking for positive infinity? It will be larger than INT32_MAX I guess.

Good point. By an infinite amount, according to mathematicians.

@saghul saghul merged commit bb14020 into master Feb 3, 2025
61 checks passed
@saghul saghul deleted the refactor-error-stacktracelimit branch February 3, 2025 10:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants