-
Notifications
You must be signed in to change notification settings - Fork 4
fix: add validation for literals #64
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
Conversation
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.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACIR Opcodes'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.01
.
Benchmark suite | Current: 9d1c441 | Previous: b494199 | Ratio |
---|---|---|---|
parse_json_from_string_JSON16kb_Bench.json/main |
1094548 acir_opcodes |
906131 acir_opcodes |
1.21 |
parse_json_from_string_JSON512b_Bench.json/main |
38027 acir_opcodes |
32545 acir_opcodes |
1.17 |
This comment was automatically generated by workflow using github-action-benchmark.
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.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Circuit Size'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.01
.
Benchmark suite | Current: 9d1c441 | Previous: b494199 | Ratio |
---|---|---|---|
parse_json_from_string_JSON16kb_Bench.json/main |
1469079 circuit_size |
1243803 circuit_size |
1.18 |
parse_json_from_string_JSON512b_Bench.json/main |
68568 circuit_size |
63403 circuit_size |
1.08 |
This comment was automatically generated by workflow using github-action-benchmark.
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.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Brillig Bytecode Size'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.01
.
Benchmark suite | Current: 02af66e | Previous: b494199 | Ratio |
---|---|---|---|
parse_json_from_string_JSON16kb_Bench |
17177 opcodes |
16999 opcodes |
1.01 |
parse_json_from_string_JSON512b_Bench |
17168 opcodes |
16990 opcodes |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
@@ -607,6 +607,36 @@ impl<let NumBytes: u32, let NumPackedFields: u32, let MaxNumTokens: u32, let Max | |||
let index_valid: Field = range_valid[i] as Field; | |||
// 1 gate | |||
let entry = TranscriptEntry::to_field(TranscriptEntry { token, index, length }); | |||
|
|||
if token == LITERAL_TOKEN as Field { | |||
index.assert_max_bit_size::<8>(); |
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've moved this range constraint to sit before the cast to u32 to ensure that we don't truncate.
let first_char = self.json[index_as_u32]; | ||
let second_char = self.json[index_as_u32 + 1]; | ||
let third_char = self.json[index_as_u32 + 2]; | ||
let fourth_char = self.json[index_as_u32 + 3]; |
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'm pulling these array reads out to be unconditional
& (fifth_char == 101); // 'e' | ||
assert(is_false, "invalid literal"); | ||
} else { | ||
assert_eq(length, 4, "invalid literal"); |
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.
We can just assert on the length here to avoid having a predicate in this branch.
@jialinli98 I've pulled out these changes from #61 so that we can inspect these changes in isolation. |
Description
Problem*
Resolves
Summary*
This PR pulls out @jialinli98's commit in #61
Additional Context
PR Checklist*
cargo fmt
on default settings.