Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LibJS: RAII in js???? #16630

Merged
merged 8 commits into from
Jan 23, 2023
Merged

LibJS: RAII in js???? #16630

merged 8 commits into from
Jan 23, 2023

Conversation

davidot
Copy link
Member

@davidot davidot commented Dec 23, 2022

Still need to make some issues in the repo about spec problems, also the spec is only partly accepted so no async parts.

@davidot davidot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Dec 23, 2022
@konradekk
Copy link

as much as I like the code and effort, i guess that dealing with stage 2 might not be a best idea. (there was e.g. a split with stage 2 implementation of decorators being implemented once…).

iʼm not really against it but i would advise to wait for at least stage 3 proposal.

@davidot
Copy link
Member Author

davidot commented Dec 25, 2022

as much as I like the code and effort, i guess that dealing with stage 2 might not be a best idea. (there was e.g. a split with stage 2 implementation of decorators being implemented once…).

This proposal is already at stage 3, although it's not mentioned in the repo itself, but it is listed here: https://github.com/tc39/proposals under stage 3 and babel/proposals#85 (comment) this is where I got the fact that async is not stage 3.

I think some stuff will still change (I even found some spec issues) but I'm up for updating the code as the proposal gets updated.

alimpfard
alimpfard reviewed Dec 26, 2022
View reviewed changes
Copy link
Member

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

The bytecode impl is gonna be hell isn't it...

Just some random comments, nothing groundbreaking here:

Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp Show resolved Hide resolved
Userland/Libraries/LibJS/AST.cpp Show resolved Hide resolved
Userland/Libraries/LibJS/Tests/using-for-loops.js

test("using using of", () => {
let enteredLoop = false;
for (using using of [null]) {
Copy link
Member

Choose a reason for hiding this comment

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

...wtf

@stale
Copy link

stale bot commented Jan 16, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Jan 16, 2023
@ADKaster ADKaster added the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label Jan 16, 2023
@davidot davidot requested a review from linusg as a code owner January 16, 2023 21:43
@stale stale bot removed stale labels Jan 16, 2023
@davidot davidot removed the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label Jan 16, 2023
@davidot
Copy link
Member Author

davidot commented Jan 16, 2023

Addressed the comments and rebased,
also me: let's take a small break
serenity:
Screenshot from 2023-01-16 22-28-25

linusg
linusg reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Great stuff! Once again we have better test coverage than test262 😄

Userland/Libraries/LibJS/Runtime/SuppressedErrorConstructor.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/SuppressedErrorConstructor.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Tests/test-common.js Show resolved Hide resolved
Userland/Libraries/LibJS/Tests/test-common.js Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/DisposableStackConstructor.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/DisposableStackPrototype.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/DisposableStackPrototype.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/DisposableStackPrototype.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibJS/Runtime/DisposableStackPrototype.cpp Outdated Show resolved Hide resolved
@linusg linusg added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jan 20, 2023
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author labels Jan 22, 2023
@davidot
Copy link
Member Author

davidot commented Jan 22, 2023

Addressed most of the comments, just not sure what would be best in the Optional reference case

@davidot
LibJS: Clarify more errors in test-common
87bfbc9 
Without a message these just show 'ExpectationError' even if the check
has multiple steps.
@davidot
LibJS: Add custom details to toBe{True, False} shown on failure
8402726 
Any test with multiple expect(...).toBe{True, False} checks is very hard
to debug.
@davidot
LibJS: Add SuppressedError{, Prototype, Constructor}
92e69a6
@davidot
LibJS: Add an initialize binding hint to all initialize_binding methods
6d359bf 
This will allow us to specify things like SyncDispose and perhaps
AsyncDispose in the future.
@davidot
LibJS: Add Symbol.dispose
7a5e042
@davidot
LibJS: Add using declaration support, RAII like operation in js
349110d 
In this patch only top level and not the more complicated for loop using
statements are supported. Also, as noted in the latest meeting of tc39
async parts of the spec are not stage 3 thus not included.
@davidot
LibJS: Add using declaration support in for and for of loops
d39a4ca 
The using declarations have kind of special behavior in for loops so
this is seperated.
@davidot
LibJS: Add DisposableStack{, Prototype, Constructor}
04b432a 
Since the async parts of the spec are not stage 3 at this point we don't
add AsyncDisposableStack.
@davidot
Copy link
Member Author

davidot commented Jan 23, 2023

Done and also quickly checked all the commit do build and run test-js since I had some refactor trouble.

linusg
linusg approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Thanks for the initial implementation, great work! 🎉

@linusg linusg merged commit 6255ca4 into SerenityOS:master Jan 23, 2023
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jan 23, 2023
@linusg linusg mentioned this pull request Jan 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alimpfard alimpfard alimpfard left review comments

@linusg linusg linusg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidot @konradekk @alimpfard @linusg @ADKaster