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

refactor: Refactor namespace allocations using ns! macro #1132

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

huitseeker
Copy link
Member

  • This codemod was generated by the command
rustby '&mut :[cs].namespace(|| :[e])' 'ns!(:[cs], :[e])'

where rustby is an alias for
comby -matcher .rs -extensions .rs -in-place -jobs 10 -exclude-dir "target,.github" -stats -timeout 15 $@

  • Comprehensive refactoring done across multiple modules, including coprocessor/mod.rs, coroutine/memoset/env.rs, circuit/circuit_frame.rs, circuit/gadgets/pointer.rs, and many more.
  • Switched from .namespace() function to the ns! macro throughout the code to improve simplicity and readability.

@huitseeker huitseeker requested review from a team as code owners February 14, 2024 21:00
arthurpaulino
arthurpaulino previously approved these changes Feb 14, 2024
Copy link
Member

@arthurpaulino arthurpaulino left a comment

Choose a reason for hiding this comment

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

LGTM but it would be better to wait for more reviewers

adr1anh
adr1anh previously approved these changes Feb 15, 2024
Copy link
Contributor

@adr1anh adr1anh left a comment

Choose a reason for hiding this comment

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

These look good to me, it's very nice to see gadget calls fit in a single line again.

Is there anything we should be careful with regard to calling macros in macros though?

src/circuit/gadgets/macros.rs Show resolved Hide resolved
@huitseeker
Copy link
Member Author

Is there anything we should be careful with regard to calling macros in macros though?

Recursion isn't allowed, but otherwise that's a common pattern, and how zip_with in Arecibo is implemented for example.

- This codemod was generated by the command
```
rustby '&mut :[cs].namespace(|| :[e])' 'ns!(:[cs], :[e])'
```
where rustby is an alias for
`comby -matcher .rs -extensions .rs -in-place -jobs 10 -exclude-dir "target,.github" -stats -timeout 15 $@`
- Comprehensive refactoring done across multiple modules, including `coprocessor/mod.rs`, `coroutine/memoset/env.rs`, `circuit/circuit_frame.rs`, `circuit/gadgets/pointer.rs`, and many more.
- Switched from `.namespace()` function to the `ns!` macro throughout the code to improve simplicity and readability.
@huitseeker huitseeker added this pull request to the merge queue Feb 16, 2024
Merged via the queue into argumentcomputer:main with commit 4906408 Feb 16, 2024
11 checks passed
@huitseeker huitseeker deleted the switch_to_ns branch February 16, 2024 16:18
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.

None yet

4 participants