Skip to content

Alternative 2: Suffix renamings (_n suffix) #446

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

Merged
merged 2 commits into from
Nov 14, 2019
Merged

Conversation

minad
Copy link
Member

@minad minad commented Nov 5, 2019

alternative to #437

@minad minad requested a review from sjaeckel November 5, 2019 23:17
@minad minad added the finished label Nov 5, 2019
@minad minad force-pushed the suffix-renamings2 branch 2 times, most recently from bb44dd7 to b14b76c Compare November 5, 2019 23:59
@minad minad requested review from nijtmans and czurnieden November 6, 2019 07:58
@minad minad requested a review from nijtmans November 6, 2019 09:29
@minad
Copy link
Member Author

minad commented Nov 6, 2019

Is this _n suffix acceptable for you? The root function was called n_root before. I think the _d suffix should be reserved for digits.

@minad minad changed the title Suffix renamings2 Suffix renamings (_n suffix) Nov 6, 2019
@minad minad changed the title Suffix renamings (_n suffix) Alternative 2: Suffix renamings (_n suffix) Nov 6, 2019
Copy link
Collaborator

@nijtmans nijtmans left a comment

Choose a reason for hiding this comment

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

Yes, the 'n' suffix is OK to me. As type, I would prefer 'unsigned int' or 'unsigned long', since 'n' suggests a positive number: I don't want to check for n < 0 in every function, returning MP_VAL in this case. But ... that's OK for another PR

@minad
Copy link
Member Author

minad commented Nov 6, 2019

Yes, we also had the unsigned discussion in #437. Let's keep things separate.

@minad minad requested a review from czurnieden November 6, 2019 14:53
@minad minad mentioned this pull request Nov 7, 2019
@minad
Copy link
Member Author

minad commented Nov 7, 2019

@sjaeckel I guess you prefer this over alternative 1? Ready from my side.

@sjaeckel
Copy link
Member

sjaeckel commented Nov 7, 2019

@sjaeckel I guess you prefer this over alternative 1? Ready from my side.

I am not sure, that's why I left them both untouched. Does this block you?

@minad
Copy link
Member Author

minad commented Nov 8, 2019

This is not a big blocker. Maybe a bit for #430. But at at some point we have to resolve this.

@minad minad force-pushed the suffix-renamings2 branch 2 times, most recently from d2b56d0 to 793b625 Compare November 12, 2019 00:02
@minad minad force-pushed the suffix-renamings2 branch from 793b625 to 699573f Compare November 12, 2019 00:04
@sjaeckel
Copy link
Member

Okay, so you think we should go with this version?! :)

I'm fine with this version of the API.

Now let's head for 2.0 with this version as there are still some discussions upcoming e.g. regarding unsigned arguments.

@minad
Copy link
Member Author

minad commented Nov 13, 2019

I don't really care, but having it like this leaves the door open for all-mp_int functions.

@minad
Copy link
Member Author

minad commented Nov 13, 2019

This is for 2.0. but we should backport the renaming/deprecation. That's all for 1.x

@minad
Copy link
Member Author

minad commented Nov 13, 2019

Now let's head for 2.0 with this version as there are still some discussions upcoming e.g. regarding unsigned arguments.

What do you mean by that? Can this be merged?

@sjaeckel
Copy link
Member

Can this be merged?

as soon as it's rebased

@sjaeckel sjaeckel merged commit 83b74ba into develop Nov 14, 2019
@sjaeckel sjaeckel deleted the suffix-renamings2 branch November 14, 2019 10:08
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