Hi @mlocati,

Thank you for reviewing my pull request.

To handle large IP ranges efficiently, I used a generator class. This suggestion is from the IDE, recommending that the PHP version be updated to at least 5.5.

Yel, generators require PHP 5.5. But I don't think I'd increase the minimum PHP version just for this reason

@mlocati,

I have updated the code to return an array instead of a generator instance.

I'd

• remove the RangeSplitInterface interace and let any range class accept the split method (for "Range\Single" it will simply return an array with a single address)
• add tests

coverage: 98.552% (-0.07%) from 98.622%
when pulling d1f5a94 on alissn:AddSplitMethod
into 67ce14c on mlocati:main.

Hi @mlocati,

Do you have any feedback on this pull request?

Do you have any feedback on this pull request?

@alissn The automated actions already gave you a feedback 😉 - see https://github.com/mlocati/ip-lib/pull/93/checks

@alissn The automated actions already gave you a feedback 😉 - see https://github.com/mlocati/ip-lib/pull/93/checks

@mlocati
I have fixed the test.

However, PHP 5.3, 5.4, and 5.5 are no longer relevant for me and many others. 🙂
For more details, you can check the official PHP EOL page: https://www.php.net/eol.php.

@alissn I've updated the code of this pull request.
But your code only works if the range is a subnet, right? What about the other 2 types of ranges?

Also: please add some tests for the Single and Pattern classes (I don't think we need new test methods: adding cases to the two existing providers should be enough)

@mlocati
I have completed all the reviews.

However, when passing a pattern-based range to the split method, it cannot return an array of \IPLib\Range\Pattern instances. Instead, I had to return \IPLib\Range\Subnet objects.

        $systemBitness = PHP_INT_SIZE * 8;
        $minPrefixByBitness = $maxPrefix - $systemBitness + 2;
        if ($minPrefixByBitness > $networkPrefix) {
            throw new OverflowException("The value of \$networkPrefix leads to too large ranges for the current machine bitness (you can use a value of at least {$minPrefixByBitness})");
        }

I removed this section because the $networkPrefix passed to the function cannot be smaller than the current range.

Additionally, an error is already handled in the following section:

if ($networkPrefix < $myNetworkPrefix) {
    throw new OutOfBoundsException("The value of the \$networkPrefix parameter can't be smaller than the network prefix of the range ({$myNetworkPrefix})");
}

You shouldn't remove the chunk of code that throws an OverflowException: it handles the case when pow(2, ...) returns a non-integer value

Reverted the change, but I couldn't add a test for it. If you can, please add a test case to ensure the exception is thrown.

please add a test case to ensure the exception is thrown.

It's already there:

Based on the coverage report here this code path isn't covered by tests.

We send to Coveralls only one coverage, and it's for a case where we use a 64- bit PHP.
Maybe we can change that case to use a 32-bit PHP

Thanks!

See also #96 (which required #94 and #95)