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

Add JS_Eval* overloads taking line #822

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ABBAPOH
Copy link
Contributor

@ABBAPOH ABBAPOH commented Jan 12, 2025

When evaluating QML code, only parts of the file contain valid JS code; and it is crucial to pass the line of that part rather than 1 in order to report correct lines to user.

@saghul
Copy link
Contributor

saghul commented Jan 12, 2025

Can't this be implemented in user land with Error.prepareStackTrace?

This API feels quite strange.

@ABBAPOH
Copy link
Contributor Author

ABBAPOH commented Jan 12, 2025

Can't this be implemented in user land with Error.prepareStackTrace?

How so? We evaluate JS code on RHS of properties and then work with the value in C++ code.
In case of an exception, we present the stack to the user.
If you're suggesting patching the JS code by wrapping it in prepareStackTrace, that will bloat the JS code by a factor 2 to 3 as most properties contain simple expressions or plain values on RHS

@bnoordhuis
Copy link
Contributor

I think Saúl is saying (and I agree) the way the new API is designed is too niche / too special-case.

A better way is to pass in a versioned struct, e.g.:

#define JS_EVAL_OPTIONS_VERSION 1

typedef struct JSEValOptions {
  int version;
  const char *filename;
  int eval_flags;
  int line_num;
  // can add new fields in ABI-compatible manner by incrementing JS_EVAL2_VERSION
} JSEValOptions;

JS_EXTERN JSValue JS_Eval2(JSContext *ctx, const char *input, size_t input_len, JSEvalOptions *options);

And callers use that like this:

JSValue r = JS_Eval2(ctx, source, strlen(source), &(JSEvalOptions) {
    .version = JS_EVAL_OPTIONS_VERSION,
    .filename = "x.js",
    .line_num = 1,
});

JS_Eval2 should use good defaults for omitted fields, e.g.

JSValue JS_Eval2(JSContext *ctx, const char *input, size_t input_len, JSEvalOptions *options)
{
    const char *filename = "<unnamed>";
    // ...
    if (options && options->filename)
        filename = options->filename;
    // ...
}

@saghul
Copy link
Contributor

saghul commented Jan 16, 2025

Can't this be implemented in user land with Error.prepareStackTrace?

How so? We evaluate JS code on RHS of properties and then work with the value in C++ code. In case of an exception, we present the stack to the user. If you're suggesting patching the JS code by wrapping it in prepareStackTrace, that will bloat the JS code by a factor 2 to 3 as most properties contain simple expressions or plain values on RHS

Ops, forgot to reply!

WHat I meant is something like so:

  • Before evaluating some code embedded in some file, set a global symbol with the offset.
  • Set a prepareStackTrace handler than increments the lines in the traces by that offset
  • Profit?

I don't know how your app works really, so apologies if this misses the point.

Comment on lines +521 to +528
typedef struct JSEvalOptions {
int version;
const char *filename;
int eval_flags;
int line_num;
// can add new fields in ABI-compatible manner by incrementing JS_EVAL2_VERSION
} JSEvalOptions;

Copy link
Collaborator

Choose a reason for hiding this comment

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

filename should be moved to first or third position to avoid padding on 64-bit targets

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.

4 participants