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

Adds support for eexps with 12- and 20-bit addresses #825

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

zslayton
Copy link
Contributor

Prior to this PR, e-expressions with an address between 0..64 could be encoded as a single-byte opcode and larger e-expressions could only be encoded as a length-prefixed e-expression.

This patch adds read and write support for binary e-expressions that have a trailing 1- or 2-byte FixedUInt to complete the address.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@zslayton zslayton requested review from popematt and tgregg August 27, 2024 21:02
Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

🗺️ PR Tour 🧭

Comment on lines 16 to 18
EExpressionWith4BitAddress, // 0x00-0x3F -
EExpressionWith12BitAddress, // 0x40-0x4F -
EExpressionWith20BitAddress, // 0x50-0x5F -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ Of this block, these three lines (EExpressionWith*BitAddress) are the new/interesting ones. The others are highlighted because the comments were automatically realigned.

Comment on lines 400 to 404
EExpressionWith4BitAddress
| EExpressionWith12BitAddress
| EExpressionWith20BitAddress
| EExpressionWithLengthPrefix => {
ParseValueExprResult::EExp(self.read_e_expression(opcode))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ I've been considering adding a less granular OpcodeKind or similar alongside OpcodeType to capture high-level groups like EExpression or Value for places like this where we don't actually care about the encoding just yet.

Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

Overall it looks great.

src/lazy/binary/raw/v1_1/type_code.rs Outdated Show resolved Hide resolved
Comment on lines 714 to 717
&[
// Opcode with low nibble setting bias
0x50 | low_opcode_nibble as u8,
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
&[
// Opcode with low nibble setting bias
0x50 | low_opcode_nibble as u8,
],
// Opcode with low nibble setting bias
0x50 | low_opcode_nibble as u8,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extend_from_slices_copy method takes a slice-of-slices (&[&[u8]]). For potentially interesting information about the motivation for this method, check out the PR that introduced it.

Now that I'm re-reading this, though, I could rewrite it to call (biased as u16).to_le_bytes() in advance and pack the two bytes into this slice manually. I think I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that now. I missed that the call on L701 was extend_from_slice_copy whereas the call on L713 was extend_from_slices_copy.

src/lazy/encoder/binary/v1_1/value_writer.rs Outdated Show resolved Hide resolved
@zslayton zslayton merged commit 667a2a7 into main Aug 28, 2024
32 checks passed
@zslayton zslayton deleted the multibyte-macro-ids branch August 28, 2024 13:57
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