bn: fix Red#imod #178
bn: fix Red#imod #178
Conversation
fanatid
force-pushed
the
fix/red-imod
branch
from
86044e4
to
5125c8b
Compare
lib/bn.js
Outdated
| return a.umod(this.m)._forceRed(this); | ||
|
|
||
| var mod = a.umod(this.m)._forceRed(this); | ||
| if (mod !== a) mod.copy(a); |
There was a problem hiding this comment.
This might be not so good performance-wise. Any ideas?
There was a problem hiding this comment.
Should we introduce iumod instead?
There was a problem hiding this comment.
Agree that this can introduce performance issues... I thought about new method .move instead .copy.
Should we introduce
iumodinstead?
do you mean BN#iumod?
There was a problem hiding this comment.
.move might actually work great here!
There was a problem hiding this comment.
Do you want it as separate method or only as few lines in Red#imod? What about BN#iumod? It's doesn't cost it?
There was a problem hiding this comment.
@fanatid do you mean new method for .move? If so, I think yes
There was a problem hiding this comment.
BN#move added in fa40b8c
lib/bn.js
Outdated
| return a.umod(this.m)._forceRed(this); | ||
|
|
||
| var mod = a.umod(this.m)._forceRed(this); | ||
| if (mod !== a) mod.move(a); |
There was a problem hiding this comment.
is it even worth checking if they are different? (performance diff?)
There was a problem hiding this comment.
In one case mod === a. Not sure does it cost check it or not...
Line 2413 in 98ac84e
There was a problem hiding this comment.
right, if mod === a, no need to move, but, as move guarantees that a == mod, why bother verifying?
It looks like an optimization, but, IMHO serves to confuse.
There was a problem hiding this comment.
Changed to a.umod(this.m)._forceRed(this).move(a);
| @@ -262,4 +262,13 @@ describe('BN.js/Reduction context', function () { | |||
| red.prime.split(input, output); | |||
| assert.equal(input.cmp(output), 0); | |||
| }); | |||
|
|
|||
| it('imod should change host object', function () { | |||
There was a problem hiding this comment.
maybe indicate (via redIMul)
lib/bn.js
Outdated
| @@ -327,6 +327,13 @@ | |||
| dest.red = this.red; | |||
| }; | |||
|
|
|||
| BN.prototype.move = function move (dest) { | |||
There was a problem hiding this comment.
Oh, shouldn't it be private? It looks like this could lead to funny state issues if used as a part of API.
There was a problem hiding this comment.
I'll not mind of making it private. Do you want see it as separate function or as "inline" function?
There was a problem hiding this comment.
IMO, it makes sense to have it on a prototype. Just prefix it with _. @dcousens what do you think?
There was a problem hiding this comment.
@indutny I think the public interface semantics of move can be assumed that the originating object is reset or garbage after use.
I think it is fine to have on the public interface; however, I don't mind either way.
There was a problem hiding this comment.
@fanatid prefix it with _?
There was a problem hiding this comment.
@dcousens added prefix _, let me know if I should squash commits
Issue #177