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

Parse the code section #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

virgil-serbanuta
Copy link
Member

@virgil-serbanuta virgil-serbanuta commented Jan 22, 2025

Closes #61

@virgil-serbanuta virgil-serbanuta force-pushed the parse-functions-refactor branch from 1d75164 to fced21b Compare January 22, 2025 14:21
@virgil-serbanuta virgil-serbanuta changed the title Parse functions refactor Parse the code section Jan 22, 2025
@virgil-serbanuta virgil-serbanuta marked this pull request as ready for review January 22, 2025 16:15
Copy link

@sskeirik sskeirik left a comment

Choose a reason for hiding this comment

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

So, I've reviewed everything except the tests and instruction parser generator and all looks good. I left a couple minor comments.

I will finish that part of the next part of the review in a second round.

@@ -20,10 +24,11 @@ module BINARY-PARSER-INT [private]
rule bit8IsSet(I:Int) => I &Int 128 =/=Int 0

syntax Int ::= clearBit8(Int) [function, total]
rule clearBit8(I:Int) => I ^Int (I &Int 128)
rule clearBit8(I:Int) => I -Int 128 requires 128 <=Int 128

Choose a reason for hiding this comment

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

Looks like a typo --- I'm assuming you want this:

Suggested change
rule clearBit8(I:Int) => I -Int 128 requires 128 <=Int 128
rule clearBit8(I:Int) => I -Int 128 requires 128 <=Int I

module BINARY-PARSER-FUNC-SYNTAX
imports BINARY-PARSER-BASE-SYNTAX

syntax DefnResult ::= parseDefnFunc(BytesWithIndex) [function, total]

Choose a reason for hiding this comment

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

I think that this function defined here is not used anywhere; probably this code was used somewhere but then a refactor happened which made it unnecessary.

Comment on lines +68 to +70
syntax Int ::= buildLeb128UInt(Ints) [function, total]
rule buildLeb128UInt(.Ints) => 0
rule buildLeb128UInt(Value:Int L:Ints) => Value +Int 128 *Int buildLeb128UInt(L)

Choose a reason for hiding this comment

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

Might be redundant with the definition on lines 57-59

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.

Parse the code Section
2 participants