Skip to content

fix: Prevent negative zero#8511

Merged
jfecher merged 8 commits intomasterfrom
jf/comptime-negative-zero
May 21, 2025
Merged

fix: Prevent negative zero#8511
jfecher merged 8 commits intomasterfrom
jf/comptime-negative-zero

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented May 14, 2025

Description

Problem*

Resolves #8498

Summary*

Prevents negative zero from being constructed in a SignedField wherever SignedField is used. This was only a known issue for the comptime interpreter but I thought it'd be safer to make these fields private so we could have the constructor check it everywhere.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher jfecher requested a review from a team May 14, 2025 14:35
Copy link
Contributor

@github-actions github-actions bot left a 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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 185810c Previous: f4078e1 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts 118 s 97 s 1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@jfecher jfecher enabled auto-merge May 14, 2025 15:06
@rkarabut
Copy link
Contributor

@jfecher looks like SignedField needs its fields to be public?

@jfecher
Copy link
Contributor Author

jfecher commented May 21, 2025

@rkarabut those just need to be updated to use the accessor methods now instead.

Didn't know this PR didn't merge already, I'll fix it today.

Copy link
Contributor

@github-actions github-actions bot left a 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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 8e06b08 Previous: d03ec29 Ratio
rollup-base-public 15.66 s 12.42 s 1.26

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@jfecher jfecher added this pull request to the merge queue May 21, 2025
Merged via the queue into master with commit 4ee2d12 May 21, 2025
118 checks passed
@jfecher jfecher deleted the jf/comptime-negative-zero branch May 21, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

-0 != 0 in comptime

4 participants