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

feat: add the column number feature of Error Object and its stack #143

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ErosZy
Copy link

@ErosZy ErosZy commented Nov 12, 2022

What kind of change does this PR introduce?

  1. column number support;
  2. fix some line number bugs;

Does this PR introduce a breaking change?
Yes, it had added column table, which will affect the layout of bytecode.

Why submit this PR?
The column number is very important for some functions:

  1. SourceMap
  2. Debugger API (e.g. access to CDP)
  3. Error logs for developing

Does the test262 pass?
Yes, it passed. the PR does not add any new test262 failure items. We also added a bit of testing for column number, whose behavior is consistent with the other popular engines.

HarlonWang added a commit to HarlonWang/quickjs that referenced this pull request Mar 16, 2023
TooTallNate pushed a commit to TooTallNate/quickjs that referenced this pull request Dec 18, 2023
Observed in generated code for static initializers. We could in theory
track and correct it in js_parse_class() but doing it as a peephole
optimization is both easier and more general.
@richarddd
Copy link
Contributor

@bellard can this be merged now 👏

@bellard
Copy link
Owner

bellard commented Jan 13, 2024

No. Your patch is too complicated and does not correctly handle optimisations. It is much simpler and efficient to combine the line and the column numbers in OP_line_num with (line_num << N) | col_num). Your tests could be useful though.

@richarddd
Copy link
Contributor

@ErosZy would you be able to update the PR 👆

@ErosZy
Copy link
Author

ErosZy commented Jan 13, 2024

@bellard @richarddd Sorry, I'm a bit confused because a line_number may have multiple column_numbers. How should this be merged into OP_line_num? A common scenario is when code is compressed into a single line but has many columns.

@richarddd
Copy link
Contributor

@bellard @richarddd Sorry, I'm a bit confused because a line_number may have multiple column_numbers. How should this be merged into OP_line_num? A common scenario is when code is compressed into a single line but has many columns.

I know to little of QJS bytecode but cant we use a singel opcode and track them individually? Is a column really a separate OP?

GerHobbelt pushed a commit to GerHobbelt/quickjs that referenced this pull request Jun 21, 2024
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.0.10 to 5.0.12.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v5.0.12/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v5.0.12/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

3 participants