Skip to content
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

Add default expression-width field to Nargo.toml #4961

Closed
Tracked by #4960
TomAFrench opened this issue May 2, 2024 · 1 comment · Fixed by #5505
Closed
Tracked by #4960

Add default expression-width field to Nargo.toml #4961

TomAFrench opened this issue May 2, 2024 · 1 comment · Fixed by #5505
Assignees

Comments

@TomAFrench
Copy link
Member

TomAFrench commented May 2, 2024

As part of #4960 we're no longer going to be able to query the backend to see the correct expression width to use. We can still use 4 as a default but we're going to want to be able to set a new default with a key in Nargo.toml to avoid users using backends other than bb from having to specify it each time.

This entry should be ignored in any library crates. Also need to consider how workspaces are affected.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 2, 2024
@TomAFrench TomAFrench self-assigned this May 2, 2024
github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
# Description

## Problem\*

Progress on #4961 

## Summary\*

This PR removes the query to the backend in order to get the expression
width, for now we just have a blanket default of ~4~ 3 (Looks like we're
still waiting to update to a version of `bb` which uses 4). I've decided
to split the ability to specify a package-level override of this default
as I'd need to do some mucky stuff related to `compile_options`.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.
We don't have docs related to the expression width afaik.
# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@TomAFrench
Copy link
Member Author

Removing this from current sprint as the high value component of this is complete in #4975.

@TomAFrench TomAFrench assigned vezenovm and unassigned TomAFrench Jun 28, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 15, 2024
# Description

## Problem\*

Resolves #4961 

## Summary\*

This PR let's a developer override the default expression width target
with a field in the `Nargo.toml` manifest.

Due to the limitations of the `clap` macros I had to switch
`expression_width` on the `CompileOptions` struct to optional. I then
hardcoded a constant that specifies we want width 4 as Noir's
compilation default. We then settle on a target expression width by
checking the following:
1. Is there a user supplied expression width from the CLI flag? If there
is use that width.
2. Is there no user supplied width and an expression width specified in
the manifest? Use the expression width in the manifest.
3. Is there no user supplied width and no width specified in the
manifest? Use the hardcoded default value.

## Additional Context

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants