Add support for remaining private methods #9306
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
00dc865
to
d8af2bd
Compare
❌🪟 @jdalton, there are 473 test regressions on Windows x86_64
|
paperdave
reviewed
Mar 8, 2024
jdalton
force-pushed
the
jdalton/array-private-methods
branch
2 times, most recently
from
ef36e7a
to
5e9f82f
Compare
jdalton
commented
Mar 11, 2024
| if (typeof value === null) throw new Error("Invalid enum object " + name + " defined in " + import.meta.file); | ||
| const keys = Array.isArray(value) ? value : Object.keys(value).filter(k => !k.match(/^[0-9]+$/)); | ||
| define[`$${name}IdToLabel`] = "[" + keys.map(k => `"${k}"`).join(", ") + "]"; | ||
| define[`$${name}LabelToId`] = "{" + keys.map(k => `"${k}": ${keys.indexOf(k)}`).join(", ") + "}"; |
There was a problem hiding this comment.
☝️ Used the index param of map instead of keys.indexOf.
jdalton
commented
Mar 11, 2024
| if (typeof value !== "object") throw new Error("Invalid enum object " + name + " defined in " + import.meta.file); | ||
| if (typeof value === null) throw new Error("Invalid enum object " + name + " defined in " + import.meta.file); | ||
| const keys = Array.isArray(value) ? value : Object.keys(value).filter(k => !k.match(/^[0-9]+$/)); | ||
| define[`$${name}IdToLabel`] = "[" + keys.map(k => `"${k}"`).join(", ") + "]"; |
There was a problem hiding this comment.
☝️ Simplified the join as the keys weren't in need of mapping.
jdalton
commented
Mar 11, 2024
| }; | ||
|
|
||
| // ------------------------------ // | ||
|
|
||
| for (const name in enums) { | ||
| const value = enums[name]; | ||
| if (typeof value !== "object") throw new Error("Invalid enum object " + name + " defined in " + import.meta.file); | ||
| if (typeof value === null) throw new Error("Invalid enum object " + name + " defined in " + import.meta.file); | ||
| const keys = Array.isArray(value) ? value : Object.keys(value).filter(k => !k.match(/^[0-9]+$/)); |
There was a problem hiding this comment.
k.match produces an array or null. In this case we just needed a boolean so regexp.test is a better fit.
jdalton
commented
Mar 11, 2024
| global?: boolean; | ||
| } | ||
|
|
||
| /** Applies source code replacements as defined in `replacements` */ | ||
| export function applyReplacements(src: string, length: number) { | ||
| let slice = src.slice(0, length); | ||
| let rest = src.slice(length); | ||
| slice = slice.replace(/([^a-zA-Z0-9_\$])\$([a-zA-Z0-9_]+\b)/gm, `$1__intrinsic__$2`); |
There was a problem hiding this comment.
☝️ a-zA-Z0-9_ is \w. (there's case folding too but not an issue here)
jdalton
commented
Mar 11, 2024
src/codegen/replacements.ts
Outdated
| } | ||
| let match; | ||
| if ((match = slice.match(/__intrinsic__(debug|assert)$/)) && rest.startsWith("(")) { |
There was a problem hiding this comment.
☝️ Doing the cheap check first (.startsWith).
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
5e9f82f
to
7358e72
Compare
jdalton
commented
Mar 11, 2024
| for (const replacement of replacements) { | ||
| slice = slice.replace(replacement.from, replacement.to.replaceAll("$", "__intrinsic__").replaceAll("%", "$")); |
There was a problem hiding this comment.
Move replacement.to transforms higher to ensure they are performed once and to improve consistency of transformation between replacements and globalReplacements.
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
f4d5219
to
f18a4ec
Compare
jdalton
commented
Mar 11, 2024
| { | ||
| from: /\bnotImplementedIssueFn\(\s*([0-9]+)\s*,\s*((?:"[^"]*"|'[^']+'))\s*\)/g, | ||
| to: "() => $throwTypeError(`${$2} is not implemented yet. See https://github.com/oven-sh/bun/issues/$1`)", | ||
| }, |
There was a problem hiding this comment.
Before this PR the globalReplacements .to was being transformed with.replaceAll("$", "__intrinsic__") which would have made the $2 and $1 incorrectly transformed because it didn't have the transform that the replacements .to has for %1, %2, etc.
jdalton
commented
Mar 11, 2024
| export const globalReplacements: ReplacementRule[] = [ | ||
| { | ||
| from: /\bnotImplementedIssue\(\s*([0-9]+)\s*,\s*((?:"[^"]*"|'[^']+'))\s*\)/g, | ||
| to: "new TypeError(`${$2} is not implemented yet. See https://github.com/oven-sh/bun/issues/$1`)", |
There was a problem hiding this comment.
notImplementedIssue isn't used in our code ✂️ , where as notImplementedIssueFn is.
paperdave
reviewed
Mar 11, 2024
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
f18a4ec
to
a5ccb5c
Compare
jdalton
changed the title
jdalton
force-pushed
the
jdalton/array-private-methods
branch
5 times, most recently
from
8971d01
to
30b8332
Compare
Closed
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
30b8332
to
68ae23b
Compare
jdalton
force-pushed
the
jdalton/array-private-methods
branch
6 times, most recently
from
637b3d1
to
7e912a2
Compare
jdalton
commented
Mar 29, 2024
| @@ -84,7 +84,7 @@ void JSNextTickQueue::drain(JSC::VM& vm, JSC::JSGlobalObject* globalObject) | |||
|
|
|||
| if (!isEmpty()) { | |||
| if (mustResetContext) { | |||
| globalObject->m_asyncContextData.get()->putInternalField(vm, 0, jsUndefined()); | |||
| globalObject->asyncContextTuple()->putInternalField(vm, 0, jsUndefined()); | |||
| } | |||
There was a problem hiding this comment.
With this PR I've made a distinction between the asyncContextTuple and asyncContextData because in main the current forms are conflated as asyncContext which gets confusing (is it the tuple or the data of the tuple). With this we have a clear and consistent distinction.
jdalton
commented
Mar 29, 2024
| @@ -864,8 +865,8 @@ export function onPullDirectStream(controller) { | |||
| deferFlush = controller._deferFlush; | |||
| controller._deferFlush = controller._deferClose = 0; | |||
|
|
|||
| if (asyncContext) { | |||
| $putInternalField($asyncContext, 0, prev); | |||
There was a problem hiding this comment.
Fixed as prev was not defined in the outer scope.
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
7e912a2
to
dcfdfcb
Compare
jdalton
force-pushed
the
jdalton/array-private-methods
branch
2 times, most recently
from
2e79afa
to
515bd66
Compare
jdalton
force-pushed
the
jdalton/array-private-methods
branch
from
515bd66
to
8843060
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Add support for remaining private methods.
This is a follow-up to oven-sh/WebKit#46.