Skip to content

lib: restate the code to use primordials - #36561

Closed
PoojaDurgad wants to merge 1 commit into
nodejs:masterfrom
PoojaDurgad:pri-v8prof
Closed

lib: restate the code to use primordials#36561
PoojaDurgad wants to merge 1 commit into
nodejs:masterfrom
PoojaDurgad:pri-v8prof

Conversation

@PoojaDurgad

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 18, 2020
@PoojaDurgad

Copy link
Copy Markdown
ContributorAuthor

@aduh95

Copy link
Copy Markdown
Contributor

I tried doing that in #36527 (fe72118 and b30bb91), but it seems this file cannot use primordials.

@PoojaDurgad

Copy link
Copy Markdown
ContributorAuthor

@aduh95 - yeah, looks like this file cannot use primordials. but why ? is there any other files which cannot use primordials?

@aduh95

Copy link
Copy Markdown
Contributor

Not sure, but I think this is a one-of.

@Trott

Copy link
Copy Markdown
Member

Feel free to continue discussion or even re-open if you think appropriate, but I'm going to close this since it doesn't seem like it's going to land.

@TrottTrott closed this Dec 21, 2020
filtered = filtered.split('\n');
return out.replace(FUNC_RE, (all, prefix, postfix) => {
filtered = StringPrototypeSplit(filtered, '\n');
return StringPrototypeReplace(out, FUNC_RE, (all, prefix, postfix) => {

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.

Note that calling StringPrototypeReplace on a RegExp is unsafe, as it calls RegExp.prototype[Symbol.replace].


This should instead use:

Suggested change
returnStringPrototypeReplace(out,FUNC_RE,(all,prefix,postfix)=>{
returnRegExpPrototypeSymbolReplace(FUNC_RE,out,(all,prefix,postfix)=>{

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@PoojaDurgad@aduh95@Trott@ExE-Boss@nodejs-github-bot