Skip to content

Conversation

@nijtmans
Copy link
Collaborator

Noted an off-by-one error, testing for MP_RMAP_REVERSE_SIZE < pos. Explanation: If MP_RMAP_REVERSE_SIZE == pos this returns false, so the code continues to read the table. Lucky, this only happens when the char value is 128, which is impossible for signed characters. So, this bug can actually only happen on a system with unsigned characters.

Further on, I noted various other tiny mistakes, e.g. the use of SIZE_MAX in stead of the real available bffer-size. Also the s_mp_rmap_reverse table can be reduced 8 in lenght, if we start with '+' in stead of with '('. This branch contains all I found related to mp_radix stuff.

Anyway, I hope you like this clean-up.

@nijtmans nijtmans requested review from minad and sjaeckel October 30, 2019 13:25
@nijtmans
Copy link
Collaborator Author

One more remark: ((y == 0xff) || (y >= radix)). The check for y == 0xff is redundant, because the radix is already checked before to be between 2 and 64. So if y == 0xff, it will be >= radix for sure.

@minad
Copy link
Member

minad commented Oct 30, 2019

Good find!

@minad
Copy link
Member

minad commented Oct 30, 2019

You might only want to reformat the table 😁

@minad
Copy link
Member

minad commented Oct 30, 2019

And the 0xff doesn't seem redundant since there are entries in the table.

Copy link
Member

@minad minad left a comment

Choose a reason for hiding this comment

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

See above

@nijtmans nijtmans requested a review from minad October 30, 2019 13:58
@nijtmans
Copy link
Collaborator Author

And the 0xff doesn't seem redundant since there are entries in the table.

Sorry, I disagree, it's reduntant. But maybe my explanation is not described well enough.

If I'm wrong, then you should be able to trigger the ill-behavior with a new test-case. ... (the radix conversions could use some more test-cases anyway ....)

@minad
Copy link
Member

minad commented Oct 30, 2019

Ah sorry I agree with you. With reformatting I meant, reformat such that every line is 10 numbers. My eyes are a bit hurt by how it looks now.

@nijtmans
Copy link
Collaborator Author

Ah sorry I agree with you. With reformatting I meant, reformat such that every line is 10 numbers. My eyes are a bit hurt by how it looks now.

Done! Indeed, looks better now!

@minad
Copy link
Member

minad commented Oct 30, 2019

@sjaeckel Good to merge?

@nijtmans
Copy link
Collaborator Author

nijtmans commented Nov 2, 2019

@sjaeckel ping?

@sjaeckel sjaeckel merged commit c893d21 into develop Nov 5, 2019
@sjaeckel sjaeckel deleted the radix-code-cleanup branch November 5, 2019 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants