-
Notifications
You must be signed in to change notification settings - Fork 153
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
Breaking changes for the next major release #129
Comments
I also propose to create a |
Perhaps also the following could be discussed as part of this: |
Any plans regarding #35 as part of next major release ? |
@fanatid for what? |
out of curiosity... |
@fanatid both Buffer and Uint8Array were slower for the purposes of bn.js . I've experimented with it several times, and at each try it was worse. |
@fanatid additionally, allocation is a huge problem. |
oh, okay, so the main reason is performance? How long ago are you tested? |
I've tested it about 6 months ago, but feel free to give it another try. Array allocation is fast as hell, while Buffer/Uint8Array generally takes more time to create and additional time to manage (GC). Pre-allocating buffers works, but it doesn't currently fit into the scheme of things, and considering that they were slower anyway - I didn't see a reason to change APIs. |
Thank you for explanation. |
Something like this, I guess: https://gist.github.com/indutny/f37c466b4765e686b766b0b32557557c . Btw, just recalled that |
Updated gist to make results more fair. Should be started either as |
ACK, sounds good to me. I'll happily put the time in bumping some of the crypto-browserify packages up to the new changes. |
@axic how do you use |
@dcousens luckily I am not dealing with ASN.1. |
Didn't mean to close this. @dcousens what do you think about the rest of the list? Is it something that we want to address in this major bump? |
@indutny I'd pretty much like |
@indutny if you don't intend to change
Is this function intentionally public? I think the rest of the list is good, but, I don't have the time to do it, and assuming you don't, I think the changes in the last 24 hours have been fantastic. Non-strict hex has been a liability from the get-go for me. |
Yeah, I don't want to change it for exactly these reasons. Good call! Is the benefit of more sound API outweighs breaking changes? It would be better to deprecate it with a warning, but there is no code to do this yet. Oh, perhaps |
I don't mind naming it |
I suppose, but what did |
Additional breaking change request:
|
I have no idea. Should we do same renaming for -1 for hex padding. This is going to be hell of a breaking change. |
@indutny yes, but, is
I know... how about |
I think it is useful.
Maybe we can make it throw for just |
That sounds reasonable. |
Here was another potential thing: Rename |
@axic how you think, what |
Never mind, I've only realised now that |
Reviewing the release, it seems that #112 could have been more clear. Perhaps for the 6.x release then 😉 |
(Taking the discussion from #112 to here)
Proposed changes:
.modn()
return a BN (modn API inconsistency #112).strip()
to._strip()
is a breaking change too (readme: document strip() and toJSON() #105).andln()
to.andn()
(see the README). Perhaps fold the functionality intotoArrayLike
and make.andn()
safe in terms of working up to 53 bitsiusrhn()
into a specific shift right version, because it is only used inMPrime.split()
and is complex._countBits
,_zeroBits
, etc. Others could be moved too with passing aself
variable instead of using this, however that might have a speed penalty or increase?)setn
(should be renamedisetn
as it is in-place),testn
,maskn
,bincn
The text was updated successfully, but these errors were encountered: