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

feat: implement add_mul on Expression #207

Merged
merged 1 commit into from
Apr 20, 2023
Merged

feat: implement add_mul on Expression #207

merged 1 commit into from
Apr 20, 2023

Conversation

TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Apr 18, 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

This PR migrates the remaining Expression arithmetic from noirc_evaluator. The aim behind this is so that we can remove some very verbose usage of add and subtract

The add function has been changed to be the add_mul method on Expression so a + k * b can be called as a.add_mul(k, b). I'm toying with the idea that this would be better named as mul_add as we're first multiplying then adding so this may be clearer. add_mul has had some refactoring applied to improve readability.

The + and - operator implementations have been updated to use add_mul under the hood as we should always have a + b == a.add_mul(1, b) which may not be the case otherwise.

I've also implemented * which replaces the mul function from noirc_evaluator.

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.)

@TomAFrench TomAFrench requested a review from kevaundray April 18, 2023 14:27
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with moving these into acvm, I think we need to have a clean-up of the logic inside of these functions, in particular the fact that we need Ordering seems like an abstraction leak. This is not a blocker for this PR though

@kevaundray kevaundray added this pull request to the merge queue Apr 20, 2023
Merged via the queue into master with commit f156e18 Apr 20, 2023
@github-actions github-actions bot mentioned this pull request Apr 20, 2023
@TomAFrench TomAFrench deleted the add-mul branch April 20, 2023 11:01
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