Skip to content

Handle return value of JS_ToFloat64#891

Merged
saghul merged 1 commit intomasterfrom
handle-result-conv
Feb 7, 2025
Merged

Handle return value of JS_ToFloat64#891
saghul merged 1 commit intomasterfrom
handle-result-conv

Conversation

@saghul
Copy link
Copy Markdown
Contributor

@saghul saghul commented Feb 6, 2025

Fixes: #890

@saghul saghul mentioned this pull request Feb 6, 2025
Comment thread quickjs.c Outdated
Comment on lines +6768 to +6770
if (JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit) < 0) {
// Ignore error since it sets d to NAN anyway.
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe better:

Suggested change
if (JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit) < 0) {
// Ignore error since it sets d to NAN anyway.
}
// Ignore error since it sets d to NAN anyway.
// coverity[check_return]
JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit);

Clearer and, from what I remember from node's coverity scans, "using" the value like you do here doesn't work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If that works, great!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@trufae how could be test this?

@saghul saghul force-pushed the handle-result-conv branch from 228596f to 863a6bd Compare February 7, 2025 10:03
@saghul
Copy link
Copy Markdown
Contributor Author

saghul commented Feb 7, 2025

A quick github search suggests that is indeed the right way to silence it. Updated!

@trufae
Copy link
Copy Markdown
Contributor

trufae commented Feb 7, 2025

Looks good to me

@chqrlie
Copy link
Copy Markdown
Collaborator

chqrlie commented Feb 7, 2025

In case an exception is thrown by JS_ToFloat64 does the pending exception ever pose a problem? Can this conversion fail? Shouldn't we clear the exception if thrown?

@saghul
Copy link
Copy Markdown
Contributor Author

saghul commented Feb 7, 2025

In case an exception is thrown by JS_ToFloat64 does the pending exception ever pose a problem? Can this conversion fail? Shouldn't we clear the exception if shown?

We restore the exception right after the dance, so it will be discarded.

@saghul saghul merged commit 882f885 into master Feb 7, 2025
@saghul saghul deleted the handle-result-conv branch February 7, 2025 12:04
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.

Coverity scan

4 participants