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

feat: concat3 #7200

Closed
wants to merge 5 commits into from
Closed

feat: concat3 #7200

wants to merge 5 commits into from

Conversation

iAmMichaelConnor
Copy link
Collaborator

@iAmMichaelConnor iAmMichaelConnor commented Jan 27, 2025

Description

This builds off #7199 (hence the diff also including the concat method. I haven't learned graphite.

I might've gone a bit far with this one: an ugly method called concat3 which concatenates 3 arrays. The question then becomes: where do you stop? Where does the madness end? 4? 5? Maybe extra ones should be created through some metaprogramming magic?
Anyway, I would put concat3 to good use immediately, but I could also just use concat. If you think that instead I should just use concat and accept the slowdown due to the unnecessary for loops when doing arr1.concat(arr2.concat(arr3)), I guess I could...

Problem*

Resolves

Summary*

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.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 27, 2025
Copy link
Contributor

Copy link
Contributor

FYI @noir-lang/developerrelations on Noir doc changes.

@jfecher
Copy link
Contributor

jfecher commented Jan 27, 2025

The question then becomes: where do you stop? Where does the madness end? 4? 5? Maybe extra ones should be created through some metaprogramming magic?

Right, my initial reaction would be that concat3 seems like it'd be something that may be better fit for a library.

@iAmMichaelConnor
Copy link
Collaborator Author

Fair enough. I'll close this one, and leave the concat one open

@TomAFrench
Copy link
Member

TomAFrench commented Jan 28, 2025

To be honest I don't see any reason why the naive method of two calls to the concat function should be less efficient than concat3.

Currently in ACIR, we optimise out the intermediate array. This may not be the case currently but as long as the compiler realizes that the intermediate array is not used other to create the next, then it should be the same.

@jfecher
Copy link
Contributor

jfecher commented Jan 28, 2025

@TomAFrench I'd expect it to be roughly the same at least in acir but I would expect concat3 to be faster in brillig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants