quickjs icon indicating copy to clipboard operation
quickjs copied to clipboard

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

Open ErosZy opened this issue 3 years ago • 5 comments

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.

ErosZy avatar Nov 12 '22 16:11 ErosZy

@bellard can this be merged now 👏

richarddd avatar Jan 13 '24 08:01 richarddd

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.

bellard avatar Jan 13 '24 09:01 bellard

@ErosZy would you be able to update the PR 👆

richarddd avatar Jan 13 '24 11:01 richarddd

@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.

ErosZy avatar Jan 13 '24 13:01 ErosZy

@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?

richarddd avatar Jan 13 '24 21:01 richarddd