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

Proxies 2 #89

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Proxies 2 #89

merged 1 commit into from
Sep 25, 2024

Conversation

virgil-serbanuta
Copy link
Member

@virgil-serbanuta virgil-serbanuta commented Sep 20, 2024

Closes #44

@virgil-serbanuta virgil-serbanuta force-pushed the proxies-2 branch 3 times, most recently from 042c96c to 7d3a018 Compare September 23, 2024 16:41
@virgil-serbanuta virgil-serbanuta marked this pull request as ready for review September 23, 2024 16:41
@virgil-serbanuta virgil-serbanuta force-pushed the proxies-2 branch 12 times, most recently from 6474048 to 0a4ed32 Compare September 24, 2024 09:05
</k>
<trait-path> Trait </trait-path>
<method-name> #token("execute_on_dest_context", "Identifier") </method-name>
[priority(50)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

50 is the default priority of every rule. I don't think this line is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this priority number is used in other parts of the mx-rust-semantics, such as in mx-rust-semantics/main/preprocessing/methods.md, but in previous commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but I am using this to make the relation with the rule below more obvious. Maybe this is too much, so I'll send a cleanup PR if you think it should be removed.

syntax Expression ::= concatString(Expression, Expression) [seqstrict]
| toString(Expression) [strict]
| rawValue(Value)
| SemanticsError // TODO: Remove.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't this be removed now? Is this not supposed to be a part of the mx-semantics part?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do this when sending the cleanup PR, but it would require a few hours of work, so I wanted to postpone it until after the deadline.

To be precise, this is related to another TODO in this PR, the one in methods.md at line 280 - those functions use the SemanticsError as an expression. To fix both of them, I would need to make those functions similar to the ones in rust-semantics/conversions.md, returning an ...OrError result. The problem is that I would also need to change the rule above, in rules.md, at line 267. I would practically need to split the rule in two, the first rule would just rewrite to a new constructor which would also hold the result of paramsToMaybeTupleElements, and the second one would actually implement the rule.

If you think it's important to fix this now, I'll send a follow-up PR.

@virgil-serbanuta virgil-serbanuta merged commit 77eb689 into main Sep 25, 2024
1 check passed
@virgil-serbanuta virgil-serbanuta deleted the proxies-2 branch September 25, 2024 08:42
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.

Implementations - Proxies - Rust + Mx
2 participants