Add support for bcmath extension #68
Conversation
The implementations are basic, designed only for the narrow needs of the code within the project.
Modify the Base128 and Integer code to rely on the implementation agnositc number class.
Makes it more obvious what can be stored, and move it out of the ASN1 folder as it is not actually part of that implementation, just a behind the scenes detail.
Implement tests for both implemementations, and fix an inconsistency in how modulus is calculated for the bcmath implemementation.
Removed GMP extension from composer requirements. Unofrutnatley, there is not a way to specify alternatives in composer requirements.
|
It looks like you're using tabs instead of spaces. Its usually best practice not to mix conventions throughout a project, as it can be a blocker for merging even if the code is functionally fine. A minor point to make but GMP really is much faster! Not opposed to introducing bcmath support in any case, but perhaps a suggestion in the composer file (it pops up during the install) to try use gmp if possible? Looks there's something wrong with coveralls, I'm sure there's a stable version available? Anyway, code looks good, and is fairly well covered. Looks like you could remove the duplicate declaration of isNegative? It's on the base class, but overrided on each implementation - it should be abstract, or, remove the implementation since |
Update coveralls dependency, add GMP as a suggestion in the Composer file and remove the generic isNegative implementation.
|
@afk11 Oops, good catch regarding the tabs. Thanks for the other feedback, I have incorporated it:
|
| $a = BigInteger::create(-8888); | ||
| $this->assertSame(-8888, $a->toInteger()); | ||
|
|
||
| $a = BigInteger::create(PHP_INT_MAX); |
There was a problem hiding this comment.
What happens if you do it with this?
$a = BigInteger::create('123897123987123971293871298371293918237812739127398');
It's just as likely a codepath could use toInteger on such a number - maybe the implementations should refuse if the value is above PHP_INT_MAX (an exception)?
Not sure of the effect of calling the functions as it is - would it truncate to PHP_INT_MAX? Maybe a test case?
There was a problem hiding this comment.
I think the gmp_intval truncates to PHP_INT_MAX. I am of two minds about it.
Originally, I figured that it is up to the caller to only as for an integer when it can be represented. I added some checks and will now throw an exception, but such checks may be overkill and introduce a performance hit.
|
@nathanntg thanks! on another note, I wonder what your thoughts are applying return type hints? Functions documenting |
Ensure properly formatted string, when passing a string in. Separate code path for integers and strings, just to be more explicit in types. Throws an exception if receiving an invalid string to the create method. Throws an exception if converting to integer, and the value can not fit in a PHP integer.
|
Makes sense, I went ahead and switched the type hints to be explicit ( |
|
Hey @nathanntg thanks for the PR and @afk11 for the first review. As usual I'm kept quite busy during the day but I want to take a look this weekend. Since I am not doing any active PHP development anymore I will first have to setup everything. I was wondering if @afk11 would be interested in commit rights to the repository so we can streamline pull requests a bit and also open up future development of the project. Any thoughts on this? |
|
hey @fgrosse! I'd be happy to help with maintaining things - thanks for the offer! I have had some experience contributing to and using this library, though I am certainly no expert on ASN.1. That said - extra commit rights does hedge against the bus factor, and I can certainly keep things moving as more people work on the project. It's happens I've been in a similar position with the phpecc project, where I generally just help PR's get merged, maintain a release schedule as required, occasional light touch maintenance, and aim to avoid BC breaks since we don't have time for that :) |
|
Ok, so it took me a bit longer than expected but I finally took the time to review this change. I'm gonna merge this as version Thanks again for the contribution and thanks for afk11 for the review :) |
Add support for bcmath extension Merges pull request #68 by Nathan Perkins
Thanks for creating this project! I've been using this for a project of mine, and my server does not have the GMP extension. Once I saw that the extension usage was limited to a few pieces of the project, I created a fork that abstracted away the big integer operations allowing use of
bcmathas an alternative (and potentially others down the line).I wanted to offer this back to the project, in case it would benefit others. I totally understand if you want to keep the project simple and avoid introducing the alternative implementations.
The new big integer math is all handled by a
BigIntegerclass that is implementation agnostic. It returns subclasses that use an appropriate engine, determined at run time. EachBigIntegerclass is immutable and offers an OOP where operations return new instances.I added tests for the
BigIntegerclass, and it will test both the GMP and bcmath codepaths if the appropriate extensions are present.In addition, I updated the README and composer file. Unfortunately, I don't think composer has a good way to say require this or this, so I just removed the GMP requirement. (In theory, there could be a pure PHP implementation of
BigInteger, but that is probably overkill?)If this is something you are interested in merging into the project, let me know and I am happy to make further changes.