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

Add DeserializeOrigin to LLVMOrigin rewriter #1007

Merged
merged 6 commits into from
Apr 18, 2023
Merged

Add DeserializeOrigin to LLVMOrigin rewriter #1007

merged 6 commits into from
Apr 18, 2023

Conversation

Drevanoorschot
Copy link
Contributor

This pull request adds a rewriter that transforms DeserializeOrigin to LLVMOrigin.

An issue right now is that i think the type checker is ran before any rewriter is. This means by the time type check errors are reported, the DeserializeOrigin is used which is not very descriptive.

I'm not sure if and how this could be fixed.

src/col/vct/col/origin/Origin.scala Outdated Show resolved Hide resolved
@Drevanoorschot
Copy link
Contributor Author

I corrected the style and naming issues. Regarding the aforementioned type check issue: Perhaps it would be possible to make ColLLVMParser invoke the DeserializeLLVMOrigin rewriter? It would make sure the origins are converted before any error could be raised.

@Drevanoorschot
Copy link
Contributor Author

I first need to make some changes in VCLLVM before i can test whether the changes work as intended (which will take time), but at least it compiles. Seeing how this PR has been open for a while, it might be best to merge it now before the merge conflicts get complicated to solve. But that call is for @niomaster to make.

@pieter-bos
Copy link
Member

Sure, but please address the review comments:

  • There are still many formatting changes in Origin.scala in the diff, please back them out;
  • the key member of DeserializeOriginToLLVMOrigin shoud be short, and should start with a lowercase letter.

pieter-bos
pieter-bos previously approved these changes Apr 18, 2023
@pieter-bos pieter-bos marked this pull request as ready for review April 18, 2023 12:59
@pieter-bos pieter-bos merged commit 784df56 into dev Apr 18, 2023
@pieter-bos pieter-bos deleted the vcllvm_pr6 branch April 18, 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.

3 participants