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

Branches Proof Of Concept #174

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Branches Proof Of Concept #174

wants to merge 15 commits into from

Conversation

damnMeddlingKid
Copy link
Member

@damnMeddlingKid damnMeddlingKid commented Feb 28, 2022

What

This PR implements a proof of concept for handling branches in the Liquid VM.

How

OP Codes

To implement branches i've added 3 OP codes:

  • OP_EVAL_CONDITION: This operation has a constant which is a Liquid Condition object. When executed it evaluates the Condition and pushes the RTEST of the result onto the stack
  • OP_BRANCH_UNLESS: Pops a value off the stack, if its not true then jump to the offset located in the lower 16bits of the instruction.
  • OP_BRANCH: This is an unconditional branch, it's used to exit out of a block to the "endif".

Process

When an if tag is encountered we add an OP_EVAL_CONDITION for the if statement and add an OP_BRANCH_UNLESS operation that jumps to the next clause (or endif)

Parsing then resumes as normal by calling internal_block_body_parse, when internal_block_body_parse returns with an unknown tag we check if its one of the clauses we can handle like else if so we add an unconditional OP_BRANCH which exits to the next endif and continue parsing the tokens for the else block.

Nesting is handled through recursion.

Heres an example of a liquid if block and its corresponding bytecode.

image

Theres a more complete example in test.rb with elsif and nesting.

Limitations of the POC

I'm sure theres a lot thats missing, heres the stuff im aware of:

  • Currently i've only parsed conditions with a single binary comparison, this needs to be extended to handle more comparisons
  • In order to support multiple elsif clauses i've added a small array that can handle 10 clauses, this needs to be dynamically sized.
  • Theres no validation of the structure of the tag. right now its possible to have an else before an elsif
  • No restriction on the nesting depth


uint8_t* instruction;

ptrdiff_t exit_branches[10];
Copy link
Member Author

Choose a reason for hiding this comment

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

This array is used to keep track of all the branches that need to jump to "endif". This needs to use a dynamically sized list here, something like cbuffer.

Copy link
Member Author

Choose a reason for hiding this comment

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

The offsets get stored with the instruction in 16 bits. assuming this is ok for a jump offset, otherwise need to store them somewhere else.

@@ -262,6 +282,112 @@ static tag_markup_t internal_block_body_parse(block_body_t *body, parse_context_
return unknown_tag;
}

VALUE parse_single_binary_comparison(VALUE markup) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Only supports parsing 1 binary comparison, need to extend this to support more conditions.

*exit_end++ = vm_assembler_open_branch(body_code, OP_BRANCH);

// Calculate the offset that would jump to here, this is where the <if> jumps to if it fails the condition.
jump = (ptrdiff_t) (body_code->instructions.data_end - body_code->instructions.data);
Copy link
Member Author

Choose a reason for hiding this comment

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

Calculating indices since cbuffer can realloc.


// Resolve the open branch from the <if> with the calculated offset.
vm_assembler_close_branch(body_code, open_branch, jump);
open_branch = -1;
Copy link
Member Author

Choose a reason for hiding this comment

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

using -1 as a sentinel value to indicate no open branch.

render_score_increment += 1;
body->as.intermediate.blank = false;
break;
} else if (
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of scrappy, need a better abstraction for this.

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 was thinking of something along the lines of #96 so we could pick the right parsing handler from ruby land.

Comment on lines +223 to +224
render_score_increment += 1;
body->as.intermediate.blank = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what these two do, might be updating them incorrectly.

@damnMeddlingKid damnMeddlingKid changed the title Franklyn/reviewable Branches Proof Of Concept Mar 1, 2022
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.

None yet

1 participant