Format improvements (WIP) #63
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -554,18 +554,25 @@ fn encode_args( | |
| let mut param_mask: raw::ParamMask = 0; | ||
| let mut current_param_mask_bit: raw::ParamMask = 1; | ||
|
|
||
| let mut small_padding_value = 0i8; | ||
|
Owner
There was a problem hiding this comment. I want to see tests for:
That last one would involve
truth/tests/integration_impl/mod.rs Line 227 in c41358e |
||
|
|
||
| // Important: we put the shortest iterator (args_iter) first in the zip list | ||
| // to ensure that this loop reads an equal number of items from all iters. | ||
| assert!(args_iter.len() <= arg_encodings_iter.len()); | ||
| for enc in arg_encodings_iter.by_ref() { | ||
| if let ArgEncoding::Padding { size } = enc { | ||
| match size { | ||
| 1 => { | ||
| if (args_blob.position() & 3) == 0 { small_padding_value = 0i8 } | ||
|
Owner
There was a problem hiding this comment. Please avoid one-line ifs. I reserve these for extreme cases (usually only when there are multiple similar ifs in a row and I want to emphasize the similarities between them)
Contributor
Author
There was a problem hiding this comment. I'll freely admit the entire logic for handling that was a 5 minute quick fix that bolts a random functionality where it doesn't quite belong. I'd love to replace the logic with something better that doesn't feel as dirty.
Owner
There was a problem hiding this comment. As the complexity of this function increases (which it is no doubt going to, without end), we're going to want to start factoring out pieces of it into separable units. This thing right here is easily separated out into a Finite State Machine. You could have something like: /// Finite-state machine helper for the value of `-` padding.
struct SmallPaddingFillHelper {
fill_value: i8,
}
impl SmallPaddingFillHelper {
/// Update the helper in response to the next arg encoding.
///
/// Returns the current fill value for `-` padding.
fn step(&mut self, enc: &ArgEncoding, blob_position: u64) -> i8 {
todo!("update self.fill_value");
self.fill_value
}
}and then you could put
Contributor
Author
There was a problem hiding this comment. I meant more that I'd rather leave the padding bytes as exclusively 0, eliminate the padding fill variable entirely, and let the values be handled by a |
||
| args_blob.write_i8(small_padding_value).expect("Cursor<Vec> failed?!") | ||
| }, | ||
| 4 => args_blob.write_u32(0).expect("Cursor<Vec> failed?!"), | ||
| _ => unreachable!(), | ||
| } | ||
| continue; | ||
| } | ||
| small_padding_value = 0i8; | ||
|
|
||
| let arg = args_iter.next().expect("function arity already checked"); | ||
|
|
||
| let arg_bit = match &arg.value { | ||
|
|
@@ -607,14 +614,28 @@ fn encode_args( | |
|
|
||
| | ArgEncoding::JumpOffset | ||
| | ArgEncoding::JumpTime | ||
| => args_blob.write_i32(arg.expect_raw().expect_int()).expect("Cursor<Vec> failed?!"), | ||
|
|
||
| | ArgEncoding::Integer { size: 4, extend, format: ast::IntFormat { unsigned: false, radix: _ }, .. } | ||
| => { | ||
| let value = arg.expect_raw().expect_int(); | ||
| if extend && value < 0 { small_padding_value = -1 } | ||
| args_blob.write_i32(value).expect("Cursor<Vec> failed?!") | ||
| }, | ||
|
|
||
| | ArgEncoding::Integer { size: 2, extend, format: ast::IntFormat { unsigned: false, radix: _ }, .. } | ||
| => { | ||
| let value = arg.expect_raw().expect_int(); | ||
| if extend && value < 0 { small_padding_value = -1 } | ||
| args_blob.write_i16(value as _).expect("Cursor<Vec> failed?!") | ||
| }, | ||
|
|
||
| | ArgEncoding::Integer { size: 1, extend, format: ast::IntFormat { unsigned: false, radix: _ }, .. } | ||
| => { | ||
| let value = arg.expect_raw().expect_int(); | ||
| if extend && value < 0 { small_padding_value = -1 } | ||
| args_blob.write_i8(value as _).expect("Cursor<Vec> failed?!") | ||
| }, | ||
|
|
||
| | ArgEncoding::Integer { size: 4, format: ast::IntFormat { unsigned: true, radix: _ }, .. } | ||
| => args_blob.write_u32(arg.expect_raw().expect_int() as _).expect("Cursor<Vec> failed?!"), | ||
|
|
||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -593,7 +593,7 @@ impl AtomRaiser<'_, '_> { | |
| let pseudo_arg0 = match instr.pseudo_arg0 { | ||
| None | Some(0) => None, | ||
| Some(arg0) => { | ||
| let enc = ArgEncoding::Integer { size: 2, ty_color: None, arg0: true, immediate: true, extend: false, format: ast::IntFormat { unsigned: false, radix: ast::IntRadix::Dec } }; | ||
| let expr = self.raise_arg(emitter, &SimpleArg::from(arg0 as i32), &enc, dest_label)?; | ||
| Some(expr) | ||
| } | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extend" is too short of a name for something that is probably only ever going to be used once. Probably want something with multiple words, meaning we might need to pick now whether we want
snake_caseorkebab-caseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There're quite a few places where the game engines will read a full dword from the signature in order to check for variables but then only read the lower bits of the resulting value. Ideally
extendcould be replaced with a more general purpose attribute applicable to those situations as well since that would eliminate the need for extended padding anyway. Maybe this could be written asE(imm;range="s")or something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking this should probably be a dword-sized value with an attribute that limits its value size. However, I really do not want to see abi letters get used inside string literals.