Skip to content

Alternative 2: Suffix renamings (_n suffix)#446

Merged
sjaeckel merged 2 commits into
developfrom
suffix-renamings2
Nov 14, 2019
Merged

Alternative 2: Suffix renamings (_n suffix)#446
sjaeckel merged 2 commits into
developfrom
suffix-renamings2

Conversation

@minad

@minad minad commented Nov 5, 2019

Copy link
Copy Markdown
Member

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 czurnieden and nijtmans November 6, 2019 07:58
Comment thread doc/bn.tex
@minad minad requested a review from nijtmans November 6, 2019 09:29
@minad

minad commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

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

@nijtmans nijtmans left a comment

Copy link
Copy Markdown
Collaborator

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

minad commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

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

Comment thread mp_root_n.c
Comment thread mp_root_n.c
@minad minad requested a review from czurnieden November 6, 2019 14:53
@minad minad mentioned this pull request Nov 7, 2019
@minad

minad commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

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

@sjaeckel

sjaeckel commented Nov 7, 2019

Copy link
Copy Markdown
Member

@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

minad commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

minad commented Nov 13, 2019

Copy link
Copy Markdown
Member Author

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

@minad

minad commented Nov 13, 2019

Copy link
Copy Markdown
Member Author

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

@minad

minad commented Nov 13, 2019

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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