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.

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

Extend concrete evaluation #101

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Extend concrete evaluation #101

wants to merge 34 commits into from

Conversation

joaomhmpereira
Copy link
Member

@joaomhmpereira joaomhmpereira linked an issue Mar 28, 2024 that may be closed by this pull request
@filipeom
Copy link
Member

filipeom commented Apr 1, 2024

Hey João, sorry for the massive delay. However, in an effort to reduce your workload, I've updated issue #99. I believe we've completed most of the operators, but please pay special attention to the ones annotated with ❗, as I'm not sure if we've included them yet.

In this PR, you can remove the float operators you added. I've begun moving them directly to ecma-sl here: formalsec/ECMA-SL#stdlib

Regarding the string operators you included, I'd like to keep them, excluding the lowercase/uppercase functions. We'll also transfer those to ecma-sl's standard library.

lib/value.ml Outdated Show resolved Hide resolved
Copy link
Member

@filipeom filipeom left a comment

Choose a reason for hiding this comment

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

By the way, could you add simple tests to test_{unop|binop|triop|cvtop}, similar to the ones already present, just to verify if the construction of the operators you added is being correctly simplified in practice?

I don't want to rush this. I'm going to pin this branch to https://github.com/formalsec/ECMA-SL/tree/stdlib and see if we can start the value migration

@joaomhmpereira joaomhmpereira marked this pull request as ready for review May 3, 2024 22:54
@joaomhmpereira
Copy link
Member Author

@filipeom I think this is finally ready for review

lib/eval.ml Outdated Show resolved Hide resolved
lib/eval.ml Outdated Show resolved Hide resolved
lib/eval.ml Outdated Show resolved Hide resolved
lib/eval.ml Outdated Show resolved Hide resolved
lib/expr.ml Outdated Show resolved Hide resolved
lib/expr.ml Outdated Show resolved Hide resolved
lib/expr.ml Outdated Show resolved Hide resolved
lib/ty.ml Outdated Show resolved Hide resolved
lib/ty.ml Outdated Show resolved Hide resolved
@filipeom
Copy link
Member

Sorry for the delay, I did a quick check of the code, I'll try to give more feedback as I start to get more time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants