Sort Instruction variants + Ensure each holds a singular oparg type
#6312
Instruction enum. Variants hold 1 element
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -549,263 +549,146 @@ pub type NameIdx = u32; | |||||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||||||
| #[repr(u8)] | ||||||
| pub enum Instruction { | ||||||
| BeforeAsyncWith, | ||||||
| BinaryOperation(Arg<BinaryOperator>), | ||||||
| BinaryOperationInplace(Arg<BinaryOperator>), | ||||||
| BinarySubscript, | ||||||
| Break(Arg<Label>), | ||||||
| BuildList(Arg<u32>), | ||||||
| BuildListFromTuples(Arg<u32>), | ||||||
| BuildMap(Arg<u32>), | ||||||
| BuildMapForCall(Arg<u32>), | ||||||
| BuildSet(Arg<u32>), | ||||||
| BuildSetFromTuples(Arg<u32>), | ||||||
| /// When holds `true` it will build a slice with a third step argument. | ||||||
| BuildSlice(Arg<bool>), | ||||||
| BuildString(Arg<u32>), | ||||||
| BuildTuple(Arg<u32>), | ||||||
| BuildTupleFromIter, | ||||||
| BuildTupleFromTuples(Arg<u32>), | ||||||
| CallFunctionEx(Arg<bool>), | ||||||
| CallFunctionKeyword(Arg<u32>), | ||||||
| CallFunctionPositional(Arg<u32>), | ||||||
| CallIntrinsic1(Arg<IntrinsicFunction1>), | ||||||
| CallIntrinsic2(Arg<IntrinsicFunction2>), | ||||||
| CallMethodEx(Arg<bool>), | ||||||
| CallMethodKeyword(Arg<u32>), | ||||||
| CallMethodPositional(Arg<u32>), | ||||||
| CompareOperation(Arg<ComparisonOperator>), | ||||||
| /// Performs `in` comparison, or `not in` if `invert` is 1. | ||||||
| ContainsOp(Arg<Invert>), | ||||||
| Continue(Arg<Label>), | ||||||
| CopyItem(Arg<u32>), | ||||||
| DeleteAttr(Arg<NameIdx>), | ||||||
| DeleteDeref(Arg<NameIdx>), | ||||||
| DeleteFast(Arg<NameIdx>), | ||||||
| DeleteGlobal(Arg<NameIdx>), | ||||||
| DeleteLocal(Arg<NameIdx>), | ||||||
| DeleteSubscript, | ||||||
| DictUpdate(Arg<u32>), | ||||||
| EndAsyncFor, | ||||||
| /// Marker bytecode for the end of a finally sequence. | ||||||
| /// When this bytecode is executed, the eval loop does one of those things: | ||||||
| /// - Continue at a certain bytecode position | ||||||
| /// - Propagate the exception | ||||||
| /// - Return from a function | ||||||
| /// - Do nothing at all, just continue | ||||||
| EndFinally, | ||||||
| /// Enter a finally block, without returning, excepting, just because we are there. | ||||||
| EnterFinally, | ||||||
| ExtendedArg, | ||||||
| ForIter(Arg<Label>), | ||||||
| FormatValue(Arg<ConversionFlag>), | ||||||
| GetAIter, | ||||||
| GetANext, | ||||||
| GetAwaitable, | ||||||
| GetIter, | ||||||
| GetLen, | ||||||
| /// from ... import ... | ||||||
| ImportFrom(Arg<NameIdx>), | ||||||
| /// Importing by name | ||||||
| ImportName(Arg<NameIdx>), | ||||||
| /// Importing without name | ||||||
| ImportNameless, | ||||||
| /// Performs `is` comparison, or `is not` if `invert` is 1. | ||||||
| IsOp(Arg<Invert>), | ||||||
| Jump(Arg<Label>), | ||||||
| /// Peek at the top of the stack, and jump if this value is false. | ||||||
| /// Otherwise, pop top of stack. | ||||||
| JumpIfFalseOrPop(Arg<Label>), | ||||||
| /// Performs exception matching for except. | ||||||
| /// Tests whether the STACK[-2] is an exception matching STACK[-1]. | ||||||
| /// Pops STACK[-1] and pushes the boolean result of the test. | ||||||
| JumpIfNotExcMatch(Arg<Label>), | ||||||
| /// Peek at the top of the stack, and jump if this value is true. | ||||||
| /// Otherwise, pop top of stack. | ||||||
| JumpIfTrueOrPop(Arg<Label>), | ||||||
| ListAppend(Arg<u32>), | ||||||
| LoadAttr(Arg<NameIdx>), | ||||||
| LoadBuildClass, | ||||||
| LoadClassDeref(Arg<NameIdx>), | ||||||
| LoadClosure(Arg<NameIdx>), | ||||||
| /// Holds an index into constants vec. | ||||||
| LoadConst(Arg<u32>), | ||||||
| LoadDeref(Arg<NameIdx>), | ||||||
| LoadFast(Arg<NameIdx>), | ||||||
| LoadGlobal(Arg<NameIdx>), | ||||||
| LoadMethod(Arg<NameIdx>), | ||||||
| LoadNameAny(Arg<NameIdx>), | ||||||
| MakeFunction, | ||||||
| MapAdd(Arg<u32>), | ||||||
| MatchClass(Arg<u32>), | ||||||
| MatchKeys, | ||||||
| MatchMapping, | ||||||
| MatchSequence, | ||||||
| Nop, | ||||||
| Pop, | ||||||
| PopBlock, | ||||||
| PopException, | ||||||
| /// Pop the top of the stack, and jump if this value is false. | ||||||
| PopJumpIfFalse(Arg<Label>), | ||||||
| /// Pop the top of the stack, and jump if this value is true. | ||||||
| PopJumpIfTrue(Arg<Label>), | ||||||
| PrintExpr, | ||||||
| Raise(Arg<RaiseKind>), | ||||||
| /// Resume execution (e.g., at function start, after yield, etc.) | ||||||
| Resume(Arg<u32>), | ||||||
| ReturnConst(Arg<u32>), | ||||||
| ReturnValue, | ||||||
| Reverse(Arg<u32>), | ||||||
| SetAdd(Arg<u32>), | ||||||
| SetFunctionAttribute(Arg<MakeFunctionFlags>), | ||||||
| SetupAnnotation, | ||||||
| SetupAsyncWith(Arg<Label>), | ||||||
| SetupExcept(Arg<Label>), | ||||||
|
Member
There was a problem hiding this comment. how about at least keeping the name as comment? e.g.
Suggested change
Contributor
Author
There was a problem hiding this comment. Sure, I can do that
Member
There was a problem hiding this comment. Could you also tell me about the motivation to remove the label? They compiled to the same bytecode, but having label is easier to read the code
Contributor
Author
There was a problem hiding this comment. The motivation is the inconsistenty between opcodes that holds the same I'm not so good with words so here's what I mean: I can justify having enums like this where different variants hold different fields, even if some overlapping: enum Foo {
A { name: String, age: u8 },
B { name: String, job: String, married: bool },
...
}But I do find it redundant and overly verbose to have something like this: enum Foo {
A { name: String },
B { name: String },
C { name: String },
...
}Not to mention that we have cases like this: enum Foo {
A { target: Label },
B { end: Label },
C { handler: Label },
...
}Because enum Foo {
A(TargetLabel),
B(EndLabel),
C(HandlerLabel),
...
}
Contributor
Author
There was a problem hiding this comment. @youknowone I can revert that change if that's a deal breaker for you:)
Member
There was a problem hiding this comment. I like the new enum idea on #6313. Not sure about Label. Because all the labels are actually label. If the consistency matters, giving label for every instruction looks better to me. It feels like redundancy while writing code, but we read code more than writing code. Getting information from code is easier than checking document. The situation is also a bit different to your example. It is more like: enum Foo {
CallMethodPositional { nargs: String },
ReturnConst { idx: String },
...
}Maybe giving different types for them will also make sense though.
Contributor
Author
There was a problem hiding this comment.
IMO consistency matters, I can make them consistent in the other way around by having each Oparg to be stored as a named field if that's desired.
I think that it will, but that is out of scope for this PR, I'll do it in a separate one |
||||||
| /// Setup a finally handler, which will be called whenever one of this events occurs: | ||||||
| /// - the block is popped | ||||||
| /// - the function returns | ||||||
| /// - an exception is returned | ||||||
| SetupFinally(Arg<Label>), | ||||||
| SetupLoop, | ||||||
| SetupWith(Arg<Label>), | ||||||
| StoreAttr(Arg<NameIdx>), | ||||||
| StoreDeref(Arg<NameIdx>), | ||||||
| StoreFast(Arg<NameIdx>), | ||||||
| StoreGlobal(Arg<NameIdx>), | ||||||
| StoreLocal(Arg<NameIdx>), | ||||||
| StoreSubscript, | ||||||
| Subscript, | ||||||
| Swap(Arg<u32>), | ||||||
| ToBool, | ||||||
| UnaryOperation(Arg<UnaryOperator>), | ||||||
| UnpackEx(Arg<UnpackExArgs>), | ||||||
| UnpackSequence(Arg<u32>), | ||||||
| WithCleanupFinish, | ||||||
| WithCleanupStart, | ||||||
| YieldFrom, | ||||||
| YieldValue, | ||||||
| // If you add a new instruction here, be sure to keep LAST_INSTRUCTION updated | ||||||
| } | ||||||
|
|
||||||
| // This must be kept up to date to avoid marshaling errors | ||||||
| const LAST_INSTRUCTION: Instruction = Instruction::YieldValue; | ||||||
|
|
||||||
| const _: () = assert!(mem::size_of::<Instruction>() == 1); | ||||||
|
|
||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.