Skip to content

Conversation

@nijtmans
Copy link
Collaborator

mp_error_to_string() doesn't handle MP_ITER currently, I think it should.

@nijtmans nijtmans requested a review from sjaeckel April 12, 2019 09:09
@minad
Copy link
Member

minad commented Apr 12, 2019

While you are on it - what about replacing the function with a simple switch statement? With modern compilers you can enable coverage checking then.

Edit: Ok we are not using enums :(

@sjaeckel sjaeckel added this to the v1.1.1 milestone Apr 12, 2019
@sjaeckel
Copy link
Member

sjaeckel commented Apr 12, 2019

Edit: Ok we are not using enums :(

but we should

Edit: but not in this PR

@minad
Copy link
Member

minad commented Apr 12, 2019

No, not here, it would also give some kind of compatibility issues. This time in the public API, about which I am much more hesitant. My should was more meant as - this is how it should be done in a modern c style.

@nijtmans nijtmans requested a review from minad April 12, 2019 09:25
@nijtmans
Copy link
Collaborator Author

I agree with the comments: It should have been an enum, but that can cause compatibility issues in the public API, so doing that now is a bad idea!

@minad
Copy link
Member

minad commented Apr 12, 2019

Haha, @sjaeckel you wrote we should. I only thought it :)

@czurnieden
Copy link
Contributor

mp_error_to_string() doesn't handle MP_ITER currently, I think it should.

Yepp, forgot it, my bad.
But at least it is documented in bn.tex ;-)

And it is not really an iteration that doesn't converge in a timely manner but the list of available candidates for the coefficient a ended (significant only with MP_8BIT, although the probability is sufficiently low), so MP_ITER is another proof that I am really bad at naming things.

@sjaeckel sjaeckel merged commit a105bc9 into develop Apr 12, 2019
@sjaeckel sjaeckel deleted the missing_error_code branch April 12, 2019 11:30
@sjaeckel sjaeckel modified the milestones: v1.1.1, next Sep 9, 2019
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.

5 participants