Skip to content

Account for deprecated GetPrototypeV2() and HolderV2() in v8 14.7#6470

Open
vickykont wants to merge 1 commit intomainfrom
vasiliki/account-for-v8-deprecation
Open

Account for deprecated GetPrototypeV2() and HolderV2() in v8 14.7#6470
vickykont wants to merge 1 commit intomainfrom
vasiliki/account-for-v8-deprecation

Conversation

@vickykont
Copy link
Copy Markdown
Contributor

@vickykont vickykont requested review from a team as code owners March 31, 2026 21:22
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

Adds #if guards to replace deprecated V8 GetPrototypeV2() and HolderV2() with their renamed counterparts (GetPrototype() and Holder()) for V8 >= 14.7.

  1. [LOW] #define HolderV2 Holder in resource.h is never #undef'd. Since this is a header file, the macro leaks into every translation unit that includes it. Today this is harmless because HolderV2 is only used within resource.h itself, but it's poor hygiene and could cause surprising breakage if the token ever appears elsewhere. Adding an #undef after the last use site would tighten the scope.

This review was generated by an AI assistant and may contain inaccuracies.

// TODO(cleanup): Remove when unnecessary.
#if V8_MAJOR_VERSION >= 15 || (V8_MAJOR_VERSION == 14 && V8_MINOR_VERSION >= 7)
#define HolderV2 Holder
#endif
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This macro is never #undef'd, so it leaks into every file that includes resource.h. Consider adding #undef HolderV2 at the end of the file (after the last use at line 1233) to prevent accidental token replacement elsewhere. The GetPrototypeV2 sites avoid this problem by using #if/#else blocks at each call site — you could do the same here for consistency, but an #undef at the bottom is the minimal fix.

Suggested change
#endif
#define HolderV2 Holder
#endif // V8_MAJOR_VERSION >= 15 || (V8_MAJOR_VERSION == 14 && V8_MINOR_VERSION >= 7)

Then at the end of the file (after line ~1233, or before the closing } // namespace):

#if V8_MAJOR_VERSION >= 15 || (V8_MAJOR_VERSION == 14 && V8_MINOR_VERSION >= 7)
#undef HolderV2
#endif

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 31, 2026

I'm Bonk, and I've done a quick review of your PR.

The changes add #if preprocessor guards across four files to replace deprecated V8 14.7 APIs (GetPrototypeV2()GetPrototype(), HolderV2()Holder()), keeping the old calls for the current V8 14.6. The approach is correct and the version checks are accurate.

I posted one low-severity finding: the #define HolderV2 Holder macro in resource.h is never #undef'd, which means it leaks into all translation units that include the header. It's harmless today but should be cleaned up for good hygiene.

github run

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