Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Fix use of deprecated methods removed in V8 7.0 #124

Merged
merged 1 commit into from
Mar 17, 2019

Conversation

richardlau
Copy link
Member

StackFrame::GetFrame(uint32_t index) was deprecated in V8 6.9 and
removed in V8 7.0.

Missed a Windows only instance in 26f88d0.

Refs: #119

@richardlau
Copy link
Member Author

This is the last remaining blocker to unskipping node-report in CITGM for Node.js master/12 now that a new version of nan is out that is compatible with the version of V8 in Node.js master/12.

@richardlau richardlau mentioned this pull request Mar 15, 2019
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -212,7 +212,11 @@ static void PrintStackFromStackTrace(Isolate* isolate, FILE* fp) {
StackTrace::kDetailed);
// Print the JavaScript function name and source information for each frame
for (int i = 0; i < stack->GetFrameCount(); i++) {
Local<StackFrame> frame = stack->GetFrame(i);
Local<StackFrame> frame = stack->GetFrame(
#if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION >= 7)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are versions of V8 that don't define V8_MAJOR_VERSION?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not in any versions of Node.js this module has ever supported. It may be true in long outdated versions of Node.js (e.g. deps/v8/include/v8-version.h (where V8_MAJOR_VERSION is currently defined) doesn't exist in Node.js' v0.10 branch).

For reference, nan does similar checks: https://github.com/nodejs/nan/blob/19f08c2394b19a5dbe854020fed0446606d68955/nan.h#L215-L216

StackFrame::GetFrame(uint32_t index) was deprecated in V8 6.9 and
removed in V8 7.0.

Missed a Windows only instance in 26f88d0.

Refs: nodejs#119

PR-URL: nodejs#124
Refs: nodejs#119
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants