-
Notifications
You must be signed in to change notification settings - Fork 100
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
Nodejs v12 support #330
Nodejs v12 support #330
Conversation
V8 changed DescriptorArray from a FixedArray to a proper HeapObject. These changes update accessors for DescriptorArray fields to make them compatible with FixedArray-like and HeapObject-like access. Ref: nodejs#255
V8 changed the function name inference algorithm, which affected one of our test cases. Even if it's reverted/fixed upstream, it won't make it's way into v12, so skip that particular test for this version. Ref: https://bugs.chromium.org/p/v8/issues/detail?id=9807
Some Node.js v12 versions will have String postmortem metadata as `String__FIELD_offset__int` instead of `String__FIELD_offset__TYPE`. Handle both cases so llnode can work on more versions.
Instead of crashing if llnode fails to parse a Symbol, just return a ??? token.
cc cc @nodejs/llnode @nodejs/diagnostics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLTGM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruber stampish LGTM
Ref: nodejs/llnode#330 PR-URL: #31391 Refs: nodejs/llnode#330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
V8 changed DescriptorArray from a FixedArray to a proper HeapObject. These changes update accessors for DescriptorArray fields to make them compatible with FixedArray-like and HeapObject-like access. Ref: #255 PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
V8 changed the function name inference algorithm, which affected one of our test cases. Even if it's reverted/fixed upstream, it won't make it's way into v12, so skip that particular test for this version. Ref: https://bugs.chromium.org/p/v8/issues/detail?id=9807 PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Some Node.js v12 versions will have String postmortem metadata as `String__FIELD_offset__int` instead of `String__FIELD_offset__TYPE`. Handle both cases so llnode can work on more versions. PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Instead of crashing if llnode fails to parse a Symbol, just return a ??? token. PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #330 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Landed in 878b514...e8896e0 |
Ref: nodejs/llnode#330 PR-URL: #31391 Refs: nodejs/llnode#330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Ref: nodejs/llnode#330 PR-URL: #31391 Refs: nodejs/llnode#330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Ref: nodejs/llnode#330 PR-URL: #31391 Refs: nodejs/llnode#330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Pull Request Test Coverage Report for Build 330c52db684732798ed6adca9233a96591ae7829-PR-330Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
This PR contains all commits necessary to make llnode work on v12, plus changes to CI to run tests on v12, drop v8 and v11, and bump the major version since we're dropping support and adding new one. I made a PR on nodejs/node to update the list of postmortem constants used, but I noticed some of the constants here already changes on v13, so we'll have to work on that.