Fix/debug format #36
Fix/debug format #36
Conversation
There was a problem hiding this comment.
Thank you for the PR, this is indeed a nice idea! I have two suggestions:
Fp3is not exposed by this library, and is only used to speed-up someFp6operations internally, hence the choice of not implementing an extended set of features. As such, although a better debug information is welcome, theFp3::new()method is not needed as only for debug testing purposes, which can be ignored for this particular struct.- I believe the approach of string concatenation for both
Fp3andFp6can be made simpler and more compact/efficient by reducing the number ofwrite!calls. Have you tried iterating over the coefficients array and folding, with a custom method for writing the underlyingFpelement at each step?
src/fp3.rs
Outdated
|
|
||
|
|
src/fp6.rs
Outdated
| "{:?} + {:?}*u + {:?}*u^2 + {:?}*u^3 + {:?}*u^4 + {:?}*u^5", | ||
| self.c0, self.c1, self.c2, self.c3, self.c4, self.c5 | ||
| ) | ||
| let coeffs = [self.c0, self.c1, self.c2, self.c3, self.c4, self.c5]; |
There was a problem hiding this comment.
nit: From<Fp6> for [Fp; 6] is implemented.
src/fp6.rs
Outdated
| if first_term { | ||
| write!(f, "")?; // Handle the case where all coefficients are zero | ||
| } |
There was a problem hiding this comment.
We should not display an empty string for the zero case, it should display 0.
There was a problem hiding this comment.
Changed! Somehow I missed it
|
Also could you please rebase from latest main? I've had to update the CI to trigger jobs on your PR, sorry about that! |
RajeshRk18
force-pushed
the
fix/debug-format
branch
from
59bef73
to
47b7977
Compare
Is it fine now? |
There was a problem hiding this comment.
Thanks a bunch!
I've added a few more comments inline, basically to handle the 0 edge-case and simplify the return, otherwise looks good!
Could you just make sure the CI goes green (apart from the code coverage one). It is currently failing for rustfmt and no-std builds (some missing alloc imports). Should be good to merge after this!
| // ================================================================================================ | ||
| #[test] | ||
| fn test_debug() { | ||
| assert_eq!(format!("{:?}", Fp3::zero()), "0"); |
There was a problem hiding this comment.
Nit, but coefficients are crate-accessible, so you could have still tested the various cases you had originally, by directly calling say
Fp3 {
a0: Fp::one(),
a1: Fp::new(3),
a2: Fp::new(5),
}but again that's not a big deal as this is not part of the API.
src/fp3.rs
Outdated
| if elem_rep.is_empty() { | ||
| write!(f, "0")?; // Handle the case where all coefficients are zero | ||
| } else { | ||
| write!(f, "{}", elem_rep)?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I think it would be cleaner to handle the 0 case first, with
if *self == Fp3::zero() {
return write!(f, "0");
}so that this segment can be simply replaced by
write!(f, "{}", elem_rep)
src/fp6.rs
Outdated
| if elem_rep.is_empty() { | ||
| write!(f, "0")?; // Handle the case where all coefficients are zero | ||
| } else { | ||
| write!(f, "{}", elem_rep)?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Same comment on special case handling than Fp3. And Fp6 does implement is_zero() directly, which can then be converting to a bool.
src/fp3.rs
Outdated
| if *self == Fp3::zero() { | ||
| return write!(f, "0"); // Handle the case where all coefficients are zero | ||
| } |
There was a problem hiding this comment.
Ah sorry, maybe I wasn't being clear, but I was suggesting to have it as an initial check, at the start of the function (it is not necessary to process the list of coefficients if the element is zero).
There was a problem hiding this comment.
Makes sense. I should have done that :)
Description
Changed debug format for Fp3 and Fp6 such that coeffs of zero and one are handled.
Before the change,
Fp6::new([1, 0, 0, 1, 0, 1])is formatted as1 + 0 * u + 0 * u^2 + 1 * u^3 + 0 * u^4 + 1 * u^5.After the change, it is formatted as
1 + u^3 + u^5.PR Checklist: