Enhancement: Add missing ArgEnum fields, tweaks to immediate note #4903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
barnjamin
added
documentation
barnjamin
commented
Dec 14, 2022
barnjamin
commented
Dec 14, 2022
barnjamin
commented
Dec 14, 2022
barnjamin
commented
Dec 14, 2022
barnjamin
commented
Dec 14, 2022
Codecov Report
@@ Coverage Diff @@
## master #4903 +/- ##
==========================================
+ Coverage 54.08% 54.09% +0.01%
==========================================
Files 427 427
Lines 53528 53528
==========================================
+ Hits 28948 28958 +10
+ Misses 22314 22305 -9
+ Partials 2266 2265 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jannotti
reviewed
Dec 14, 2022
| @@ -1548,7 +1548,7 @@ For boxes that exceed 4,096 bytes, consider `box_create`, `box_extract`, and `bo | |||
|
|
|||
| ## block f | |||
|
|
|||
| - Opcode: 0xd1 {uint8 block field} | |||
| - Opcode: 0xd1 {uint8 block field index} | |||
There was a problem hiding this comment.
I'm ok with this change (and similar, above) I suppose, but don't love it. The word "index" helps to convey that the encoding here is a number that is an index into an arbitrary list of fields, defined elsewhere. Without it, a reasonable person could ask, "What do you mean, how do I encode a field?"
On the other hand, it's not as though having the word index there makes it obvious what's going on.
There was a problem hiding this comment.
open to other suggestions, option?
jannotti
approved these changes
Dec 14, 2022
This was referenced Jan 23, 2023
This was referenced Jan 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updating the
ImmediateNotefields to be a bit more consistent.Adding
ArgEnumhandler for several ops that were missing their immediate argument options.