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

Safe tactic calls for Restate #118

Merged
merged 6 commits into from
Jan 30, 2023

Conversation

sankalpgambhir
Copy link
Member

@sankalpgambhir sankalpgambhir commented Jan 10, 2023

Added unwrapTactic calls to Restate, as before this it had raw calls to kernel tactics, which subsequently raised kernel errors whenever it failed.

Also added RewriteTrue as a BasicStepTactic, since it didn't exist before, and was required for this.

@sankalpgambhir
Copy link
Member Author

I did run scalaFix T_T

@sankalpgambhir
Copy link
Member Author

Anyhow, making this a draft while I add some tests for the modified tactics as well.

@sankalpgambhir sankalpgambhir marked this pull request as draft January 10, 2023 14:30
@sankalpgambhir
Copy link
Member Author

That took less time than I expected

@sankalpgambhir sankalpgambhir marked this pull request as ready for review January 10, 2023 14:46
@SimonGuilloud
Copy link
Collaborator

SimonGuilloud commented Jan 10, 2023

Ah! I think the confusion comes from having both a Restate and a Rewrite tactic, when they do the same thing. Restate take into account both Rewrite and RewriteTrue, depending on the number of parameters, but does not catch error. Can you maybe merge the three tactic (Restate, Rewrite, RewriteTrue) into a single one with the two adequate apply functions?

@sankalpgambhir
Copy link
Member Author

sankalpgambhir commented Jan 10, 2023

Ah! I think the confusion comes from having both a Restate and a Rewrite tactic, when they do the same thing. Restate take into account both Rewrite and RewriteTrue, depending on the number of parameters, but does not catch error. Can you maybe merge the three tactic (Restate, Rewrite, RewriteTrue) into a single one with the two adequate apply functions?

The new Restate does exactly that. Would you like to remove the basic tactics Rewrite and RewriteTrue? There is some merit in keeping them separately, but not too much, I guess.

Some of the other tactics call Rewrite internally, that will change, that's all.

@sankalpgambhir
Copy link
Member Author

Funny how and when issues manifest themselves #119

@SimonGuilloud
Copy link
Collaborator

Yeah, I think keeping all three is confusing and a waste of space since they do the exact same thing. Moreover Rewrite should be renamed Restate anyway

@sankalpgambhir
Copy link
Member Author

Provided the build passes soon, we should merge this for now. Since Restate does not work in certain scenarios, we cannot remove the base versions for now. This PR makes using Restate safer in the meanwhile, and adds more tests.

@SimonGuilloud
Copy link
Collaborator

Ok let's merge that for now and I'll deal with the issue soon.

@SimonGuilloud SimonGuilloud merged commit 892c902 into epfl-lara:main Jan 30, 2023
@sankalpgambhir sankalpgambhir deleted the rewrite-tactic-checks branch April 14, 2023 14:30
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.

2 participants