Skip to content
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

Alternative 1: Suffix renamings (no suffix) #437

Closed
wants to merge 2 commits into from
Closed

Conversation

minad
Copy link
Member

@minad minad commented Oct 30, 2019

Moved out from #434.


Repeating what I wrote in #434:

Renaming some mp_(root|expt|log)_u32 functions back to suffix less version. Using uint32_t for those functions was a mistake, I have to admit that now. In particular using a suffix is not a good idea, since this is a slight obstacle now to change the types :( These functions should either take/return bitcounts (int) or mp_digits. For example mp_log is defined in terms of count_bits, so this is the natural type to use. mp_root calls mp_mul_d, so mp_digit would be a good fit. For now I am using simply int.


We still have to decide if we want a new suffix!

Furthermore we might want to discuss renaming mp_div_2d -> mp_rsh, mp_mul_2d -> mp_lsh since the 2d suffix is weird. It would also be more consistent with mp_signed_rsh. Alternatively something like mp_div_2expt might make sense. But we can also keep things as is regarding these two functions.

@nijtmans
Copy link
Collaborator

Well, I never liked the _u32 suffix, so agreed on that ;-)

For parameters that are known never to be negative, I would prefer either unsigned int or unsigned long. The difference is that unsigned long is guaranteed to contain at least 32 bit (even on MP-16BIT). So, if the API is expected to handle at least 32-bits on all platforms then unsigned long is better, otherwise unsigned int is better IMHO.

Thanks for doing this!

@minad
Copy link
Member Author

minad commented Oct 30, 2019

@nijtmans I also never liked a suffix for those functions (but I like them for the getters/setters).

Regarding unsigned - for now I want to keep it as int, like many other functions (eg mp_mul_2d etc). I want to separate the issues.
We can change this in a batch when we do #363. I see your point, but it is not without downsides. See what I wrote in #363.

@nijtmans
Copy link
Collaborator

@nijtmans I also never liked it for those functions (but I like them for the getters/setters).

Yes, for the getters/setters it's OK, agreed!

Regarding unsigned - for now I want to keep it as int, like many other functions (eg mp_mul_2d etc). I want to separate the issues.
We can change this when we do #363. I see your point, but it is not without downsides. See what I wrote in #363.

OK, understood! I'm aware of the downsides: It sure will have effect on the code, e.g. loops.

@minad minad requested a review from nijtmans October 30, 2019 09:30
@minad minad mentioned this pull request Oct 30, 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.

Looks good! B.T.W. the only function from this group used by Tcl is mp_expt_u32, so as long as this function can handle at least 28 bits for argument b it's fine for me.

@minad
Copy link
Member Author

minad commented Oct 30, 2019

@nijtmans I assume on 32/64 bit archs? Why 28 exactly? Is this some magic width in tcl due to unboxed integers in the language runtime?

@nijtmans
Copy link
Collaborator

@nijtmans I assume on 32/64 bit archs? Why 28 exactly? Is this some magic width in tcl due to unboxed integers in the language runtime?

I don't know why the limit is exactly 28, but there is a test-case in Tcl's test-suite which checks whether pow(2^28) fails. Just try in Tcl (any version):

$ tclsh8.6
% expr 2**(2**28)
exponent too large
% expr 2**(2**27)
  (now it hangs ....)

Such exponentiation takes incredibly long to calculate ... So I see the reason for limiting the exponent to 28 bits, even with 27 bits it could be used for a DOS attack on Tcl (don't tell this to any hackers group, please ....)

@czurnieden
Copy link
Contributor

Such exponentiation takes incredibly long to calculate

The exponentiation itself doesn't, it shouldn't take more than a couple of seconds, it is the number conversion that takes aeons. Even a well optimized program like Pari/GP needs over 15 seconds to convert 2^(2^27) to its over 40 million decimal digits. I don't know what our mp_to_radix would need—a couple of days maybe?

But don't worry, we are working at it in #330.

@nijtmans
Copy link
Collaborator

Indeed:

$ tclsh8.6
% expr 2**27;puts stdout
stdout
% expr 2**(2**27);puts OK
OK
%

A fraction of a second.

@minad
Copy link
Member Author

minad commented Oct 30, 2019

@sjaeckel your opinion on this? Do you have a better proposal?

@minad minad force-pushed the suffix-renamings branch 4 times, most recently from 548463c to d232821 Compare November 5, 2019 19:16
@minad minad changed the title Suffix renamings Suffix renamings (no suffix) Nov 6, 2019
@minad minad changed the title Suffix renamings (no suffix) Alternative 1: Suffix renamings (no suffix) Nov 6, 2019
doc/bn.tex Outdated Show resolved Hide resolved
@minad minad requested a review from nijtmans November 9, 2019 05:46
@minad
Copy link
Member Author

minad commented Nov 12, 2019

closed in favor of #446

@minad minad closed this Nov 12, 2019
@minad minad deleted the suffix-renamings branch November 14, 2019 14:39
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.

3 participants