-
Notifications
You must be signed in to change notification settings - Fork 853
Conversation
0728860
to
38b546d
Compare
38b546d
to
44c0c99
Compare
44c0c99
to
ac7f2fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean up! I think it's great to remove unused code to keep the codebase clean :)
At first when reviewing the PR I was a bit puzzled by all the _
prefixes added, then I realized that this is the way Rust stops complaining about unused stuff. I would suggest a different solution where possible for these two cases:
- Methods/Fields/Enums that are only used in testing, instead of prefixing them with underscore, annotate them with
#[cfg(test)]
. This applies to some getters, debug functions, etc. - Enums that have unused options: annotate just the enum with
allow_dead_code
. I believe this will look cleaner.
I think we should be more tolerant with allowing testing dead code than non-testing dead code. For example, I think it's ok to have a testing enum with options that are unused (like the StateCircuit enum to overwrite assignments). But then if an enum is used in non-testing setting and has a non-used case, perhaps it's better to remove the case (like the EVM Circuit StepLast
).
What do you think about this?
For future reference and indexing purposes (so that we can search via github), here's a list of deleted EVM math gadgets in this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a different solution where possible for these two cases:
- Methods/Fields/Enums that are only used in testing, instead of prefixing them with underscore, annotate them with #[cfg(test)]. This applies to some getters, debug functions, etc.
- Enums that have unused options: annotate just the enum with allow_dead_code. I believe this will look cleaner.
I think we should be more tolerant with allowing testing dead code than non-testing dead code. For example, I think it's ok to have a testing enum with options that are unused (like the StateCircuit enum to overwrite assignments). But then if an enum is used in non-testing setting and has a non-used case, perhaps it's better to remove the case (like the EVM Circuit StepLast).
There are reasons I like the underscore a lot more than #[cfg(test)].
and allow(dead_code)
. Generally speaking,
- underscore is a less verbose way and a very visible way to tell devs and compilers that a variable is unused.
#[cfg(test)]
comes with annoying side effects if a method depends on some imports, we have to mark that imports with#[cfg(test)]
. For example, theoverrides
property in StateCircuit depends onHashMap
, which requires us to mark#[cfg(test)]
foruse std::collections::HashMap;
. The code would look noisy in this way. Generally, I see the scattering of#[cfg(test)]
in the codebase as an anti-pattern. The desired way should be confining all test dependencies inside a module and we only mark themod testdeps
with the#[cfg(test)]
.- "allow(dead_code)" is bad because it is overkill. When we mark an enum as dead_code, we don't know if the variants are dead code or if the whole enum is dead code. Again I believe like the cfg test, the allow(dead_code) should be only applied to the module level. An unstable module that is in active development is a suitable place for allow(dead_code).
Thanks for the detailed reasoning! I didn't think about the messy imports coming from having I'm now convinced about this approach :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for taking care of this clean up :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good we cleanup quite lot of unused methods.
However, I think there are better interpretation for methods left for now comparing with just purely prefix with "_"
We can add #[allow(dead_code, reason = "XXXX")]
specificly on field/method to explain why we allow it's there. I randomly select few typical case in comments.
Add-on, to use reason=""
in allow linter, we just need to add
#![feature(lint_reasons)]
in src/lib.rs
wdyt~?
zkevm-circuits/src/evm_circuit/util/math_gadget/constant_division.rs
Outdated
Show resolved
Hide resolved
zkevm-circuits/src/evm_circuit/util/math_gadget/constant_division.rs
Outdated
Show resolved
Hide resolved
Hi @hero78119, please see my previous reasoning with Edu about why I refrain from using I like the idea of having the lint reasons feature |
Sorry, turns out the |
I read through the comment before I proposed the
For enum example, I think to clarify In my option, left a reason here is more meaningful than just prefix underscore. It's not only resolve the linter complains but also retain the meaningful insights. I dont think
but haven't finally adopted. |
f177444
to
7e2ba55
Compare
@hero78119 Thanks for expanding on the reasoning. For public or public(crate) methods, I think I'm still not convinced the same can be applied to Enum's variants. For the |
7e2ba55
to
bf11d68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review adoption on public method:) It looks really nice now 💯
Enum case is indeed overkill, and underscore prefix make more sense yes.
Thanks again for the big effort 👍
Description
#![allow(unused_imports)]
."test" flag is only used when test utils are shared within "zkevm-circuits""test" flag is gone.Issue Link
Type of change
Rationale
These are the reason I keep a method:
remainder
method of the Constant Division Gadget is such an example.match
macro for them so it is important we don't miss any variant. It makes sense to keep them even when we don't use them.I remove a method or a gadget for these reasons.
How Has This Been Tested?