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

fix: lints to guest programs #1131

Merged
merged 12 commits into from
Dec 27, 2024
Merged

fix: lints to guest programs #1131

merged 12 commits into from
Dec 27, 2024

Conversation

rkdud007
Copy link
Contributor

target to resolve #1125
q. cannot find **/test/programs so had with examples/*

@jonathanpwang
Copy link
Contributor

By **/test/programs I was referring to https://github.com/openvm-org/openvm/tree/main/extensions/algebra/tests/programs (and analogous ones for other extensions)

@rkdud007
Copy link
Contributor Author

@jonathanpwang so linting **/test/programs and examples/* and extensions/*/tests/* these 3? or should I remove examples/* now

@jonathanpwang
Copy link
Contributor

I think we can keep examples/*. So that in addition to:

find . -type d -name "programs"

./crates/toolchain/tests/programs
./extensions/pairing/tests/programs
./extensions/bigint/tests/programs
./extensions/keccak256/tests/programs
./extensions/rv32im/tests/programs
./extensions/ecc/tests/programs
./extensions/algebra/tests/programs
./benchmarks/programs

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM after adding the other **/programs directories

@rkdud007
Copy link
Contributor Author

rkdud007 commented Dec 26, 2024

Personally imo instead of allow clippy for functions like mult, have guest util crate and ppl can import it sounds nicer - also if then as its abstracted under other crate can just use iterator for have perf - but mb not scope of this PR : )

@jonathanpwang
Copy link
Contributor

jonathanpwang commented Dec 26, 2024

Some CI tests are failing; you may need to merge/rebase main, and also an issue with std

@jonathanpwang
Copy link
Contributor

Can you merge main?

@rkdud007
Copy link
Contributor Author

umm issue was prob not from main merge ( also did one) it from extensions/pairing/tests/programs/*/ which by removed unused variable lead to err on integration test, I backed up imported variables and just #![allow(unused_imports)] my this guess correct prob work now

@jonathanpwang
Copy link
Contributor

I see, I think because pairing test require feature bn254 or bls12_381. Let's hope it works now.

@jonathanpwang
Copy link
Contributor

Personally imo instead of allow clippy for functions like mult, have guest util crate and ppl can import it sounds nicer - also if then as its abstracted under other crate can just use iterator for have perf - but mb not scope of this PR : )

Yea I agree this is best practice. In this case the functions in those examples were purely one-time tests, so we don't really expect to re-use and they're not perf critical.

@jonathanpwang jonathanpwang merged commit d0eb5ba into openvm-org:main Dec 27, 2024
8 checks passed
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.

Add lints to guest programs
2 participants