Conversation
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 518db21 | Previous: 65c3939 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
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
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.
Description
Problem
This is broken out from #11710
Summary
If we want to remove
SignedFieldentirely we need a new way of representing "any kind of integer value." To do this, I've refactored the integer variants ofcomptime::Valueinto their own enum so we can use them throughout the codebase. I think this generally improves readability since it gives us a single location where our various conversions and operations like add are defined for every integer type.Our current codebase isn't always the most clear on what encoding of fields we are using:
-1 == -FieldElement::from(1)-1i8 == FieldElement::from(255)-1 == SignedField::negative(1) != SignedField::positive(-FieldElement::from(1))Although we generally use "normal form" in more places, SSA is an exception for example where we may still have fields used for any integer type but expect them to be in two's complement.
We should gradually switch over to this new
Integertype instead - so far this PR only changes comptime to use it, but it was already essentially used there already since that is where it was refactored from.Additional Context
We will want to use this for numeric generics as well to be more accurate to the actual type used. This would fix #11722 but is more difficult than just a refactor since we'd need to know which type we're creating when we create the type constants instead of later on when the numeric generic is checked against the function signature.
User Documentation
Check one:
PR Checklist
cargo fmton default settings.