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

Record source column positions #193

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Conversation

bnoordhuis
Copy link
Contributor

And:

  • display them in stack traces
  • expose them as Function.prototype.columnNumber

OP_line_num is renamed to OP_source_loc and the pc2line data structure is extended with the column number in zigzag encoding.

The bytecode version number BC_VERSION is incremented because pc2line data is read and written by JS_ReadObject() and JS_WriteObject() when it is present.

Fixes: #149

@bnoordhuis bnoordhuis requested a review from saghul December 9, 2023 22:46
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Very nice work! Random observation: I think we can safely go ahead and remove has_debug since we remove the strip mode. It'll save us a whole bit!

quickjs.c Outdated
@@ -18271,6 +18295,7 @@ static void free_token(JSParseState *s, JSToken *token)
static void __attribute((unused)) dump_token(JSParseState *s,
const JSToken *token)
{
printf("%d:%d ", token->line_num, token->col_num);
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug leftover or intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional! And it's a debug helper in the first place.

quickjs.c Outdated
@@ -18826,7 +18861,8 @@ static __exception int next_token(JSParseState *s)
if (*p == '\n') {
s->line_num++;
s->got_lf = TRUE; /* considered as LF for ASI */
p++;
s->eol = p++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance of simplifying the logic with got_lf now? I didn't look into it but there might be some overlap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not easily, I think. I can go into detail but they mean/describe slightly different things.

And:
- display them in stack traces
- expose them as Function.prototype.columnNumber

OP_line_num is renamed to OP_source_loc and the pc2line data structure
is extended with the column number in zigzag encoding.

The bytecode version number BC_VERSION is incremented because pc2line
data is read and written by JS_ReadObject() and JS_WriteObject() when
it is present.

Fixes: quickjs-ng#149
@bnoordhuis
Copy link
Contributor Author

I think we can safely go ahead and remove has_debug since we remove the strip mode.

Agreed. I'll follow up with another PR.

@bnoordhuis bnoordhuis merged commit bace4f6 into quickjs-ng:master Dec 11, 2023
@bnoordhuis bnoordhuis deleted the fix149 branch December 11, 2023 21:36
bnoordhuis added a commit to bnoordhuis/quickjs that referenced this pull request Dec 11, 2023
And merge the debug struct into JSFunctionBytecode because it is now
always present.

Refs: quickjs-ng#193 (review)
bnoordhuis added a commit to bnoordhuis/quickjs that referenced this pull request Dec 11, 2023
And merge the debug struct into JSFunctionBytecode because it is now
always present.

Refs: quickjs-ng#193 (review)
bnoordhuis added a commit that referenced this pull request Dec 11, 2023
And merge the debug struct into JSFunctionBytecode because it is now
always present.

Refs: #193 (review)
bluesky950520 pushed a commit to bluesky950520/quickjs that referenced this pull request Mar 14, 2025
And merge the debug struct into JSFunctionBytecode because it is now
always present.

Refs: quickjs-ng/quickjs#193 (review)
# 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.

Add column numbers to stack traces
2 participants