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

Fix srcMap for throw "something" #354

Closed
wants to merge 2 commits into from

Conversation

mstoykov
Copy link
Contributor

I skipped returning the call to addSrcMap in compiledAssignExpr as I don't have anything failing with it. I am currently pressed for time, so didn't really try to find one outside my tests :(

@dop251
Copy link
Owner

dop251 commented Dec 16, 2021

Hi. I deliberately removed these as emitting literal value cannot throw and I wanted to keep srcMap as small as possible. A better fix would be adding the location of the throw statement. I'll take a look shortly.

@mstoykov
Copy link
Contributor Author

I deliberately removed these as emitting literal value cannot throw and I wanted to keep srcMap as small as possible.

That makes sense and answers my question of "why?" :)

I also noticed that it does make a lot of allocations a month ago benchmarking stuff (just not enough to matter all that much IMO).

I think ddbec7f is what you want?

Post acd374c the emition of srcMap for compiledLiterals was dropped and
with that `throw "something"` suddenly stop having correct line numbers.
@mstoykov
Copy link
Contributor Author

I have redone it to actually emit on the throw. I wondered on moving that append to a function but didn't want to make bigger change than needed, and decided to leave you to decide where should it go ;)

@mstoykov mstoykov changed the title More src map fixes Fix srcMap for throw "something" Dec 17, 2021
@dop251 dop251 closed this in 6b16cd3 Dec 17, 2021
panzhongxian pushed a commit to panzhongxian/goja that referenced this pull request Dec 18, 2021
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.

2 participants