Adds OpcodeKind, LengthType to Opcode
#836
Conversation
zslayton
force-pushed
the
opcode-kind
branch
from
7152225
to
f41569d
Compare
| // TODO: Make an `OpcodeClass` enum that captures groups like this for fewer branches | ||
| if opcode.is_e_expression() || opcode.ion_type.is_some() || opcode.is_annotations_sequence() | ||
| if opcode.is_e_expression() | ||
| || opcode.ion_type().is_some() | ||
| || opcode.is_annotations_sequence() |
There was a problem hiding this comment.
🗺️ This code snippet is just a whitespace change. The deleted comment has now been implemented; opcode.is_e_expression() now involves matching against a single value instead of 4+.
| static ION_1_1_TIMESTAMP_SHORT_SIZE: [u8; 13] = [1, 2, 2, 4, 5, 6, 7, 8, 5, 5, 7, 8, 9]; | ||
| const ION_1_1_TIMESTAMP_SHORT_SIZE: [u8; 13] = [1, 2, 2, 4, 5, 6, 7, 8, 5, 5, 7, 8, 9]; |
There was a problem hiding this comment.
🗺️ static elements cannot be referenced in a const fn, but const elements can. This is effectively making a copy of this array each time it is referenced. Because it's only 13 bytes and is used once, it's not expensive.
| use LengthType::*; | ||
| match (self.ion_type_code, self.low_nibble) { | ||
| (OpcodeType::Boolean, 0xE..=0xF) => InOpcode(0), | ||
| (OpcodeType::Float, 0xA) => InOpcode(0), | ||
| (OpcodeType::Float, 0xB..=0xD) => InOpcode(1 << (self.low_nibble - 0xA)), | ||
| (OpcodeType::Integer, n) => InOpcode(n), | ||
| (OpcodeType::Nop, 0xC) => InOpcode(0), | ||
| (OpcodeType::NullNull, 0xA) => InOpcode(0), | ||
| (OpcodeType::String, 0..=15) => InOpcode(self.low_nibble), | ||
| (OpcodeType::InlineSymbol, n) if n < 16 => InOpcode(n), | ||
| (OpcodeType::SymbolAddress, n) if n < 4 => InOpcode(n), | ||
| (OpcodeType::Decimal, 0..=15) => InOpcode(self.low_nibble), | ||
| (OpcodeType::List, n) if n < 16 => InOpcode(n), | ||
| (OpcodeType::SExpression, n) if n < 16 => InOpcode(n), | ||
| (OpcodeType::TimestampShort, 0..=12) => { | ||
| InOpcode(ION_1_1_TIMESTAMP_SHORT_SIZE[self.low_nibble as usize]) | ||
| } | ||
| (OpcodeType::TypedNull, _) => InOpcode(1), | ||
| (OpcodeType::Struct, n) if n < 16 => InOpcode(n), | ||
| (OpcodeType::DelimitedContainerClose, 0) => InOpcode(0), | ||
| ( | ||
| OpcodeType::ListDelimited | ||
| | OpcodeType::SExpressionDelimited | ||
| | OpcodeType::StructDelimited, | ||
| _, | ||
| ) => Unknown, | ||
| _ => FlexUIntFollows, | ||
| } | ||
| self.length_type |
There was a problem hiding this comment.
🗺️ This code block would often cause BinaryBuffer::read_value_length to not be inlined. Now this logic happens at compile time instead.
| EExpressionWith6BitAddress | ||
| | EExpressionWith12BitAddress | ||
| | EExpressionWith20BitAddress | ||
| | EExpressionWithLengthPrefix => { | ||
| ParseValueExprResult::EExp(self.read_e_expression(opcode)) | ||
| } | ||
| AnnotationFlexSym | AnnotationSymAddress => { | ||
| let result = match opcode.kind { | ||
| OpcodeKind::EExp => ParseValueExprResult::EExp(self.read_e_expression(opcode)), | ||
| OpcodeKind::Annotations => { |
There was a problem hiding this comment.
🗺️ Notice that all of these match arms have been consolidated because we now check the kind instead of the opcode_type.
zslayton
requested review from
popematt and
nirosys
and removed request for
popematt
| @@ -55,56 +63,131 @@ impl Opcode { | |||
| /// opcode + length code combination is illegal, an error will be returned. | |||
| pub const fn from_byte(byte: u8) -> Opcode { | |||
There was a problem hiding this comment.
🗺️ This method is run at compile time to compute a u8->Opcode cache. I've augmented it to also compute the LengthType and OpcodeKind for each byte.
This PR aims to merge some optimizations that were implemented in a demo branch a while back. In particular, it:
LengthTypefor a givenOpcodefrom runtime to compile time, streamlining the process of reading a value/e-expression.OpcodeKindto theOpcodestruct, allowing the reader to check for high-level categories of syntactic elements like "value", "e-expression", or "annotations" rather than matching against all possibleOpcodeTypes in each category.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.