Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

chore!: organise operator implementations for Expression #190

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Apr 10, 2023

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Summary of changes

The disorganised approach we have to implementing various traits on Expression has been bugging me. We've got a jumble of Froms, maths operators and sorting operators all in the same file. This means that we're missing a few operator definitions which we'd want but it's not obvious that they're missing because there's not a defined order for them.

This PR takes arithmetic.rs and renames it to expression.rs, we then break this into three files:

  • mod.rs which contains the bulk of the definition of Expression along with the main impl.
  • operators.rs which defines Add, Mul, Neg, Sub on Expression against itself, Witness and FieldElement.
  • ordering.rs which contains the logic necessary to implement Ord/PartialOrd

I've also made the Add, Mul, Sub operators all commutative.

One thing to note is that I've removed the from/operator implementations for &FieldElement and &Witness in favour of FieldElement and Witness. This is to match the fact that we only define arithmetic operators for FieldElements and not &FieldElement in acir_field and Witness is a glorified u32 so the benefits of passing by reference is going to be negligible or negative.

Breaking change as we'll likely have to remove some & to account for this.

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

BEGIN_COMMIT_OVERRIDE
chore!: organise operator implementations for Expression (#190)
END_COMMIT_OVERRIDE

@kevaundray
Copy link
Contributor

I think some or all of the methods in ordering.rs won't be needed in the future, but for now, since we have them this PR generally makes sense

@kevaundray kevaundray enabled auto-merge April 18, 2023 14:04
@kevaundray kevaundray added this pull request to the merge queue Apr 18, 2023
Merged via the queue into master with commit a619df6 Apr 18, 2023
@TomAFrench TomAFrench deleted the operators branch April 18, 2023 14:11
@github-actions github-actions bot mentioned this pull request Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants