You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/06/20 06:20:25 UTC
[GitHub] [commons-numbers] arturobernalg opened a new pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
arturobernalg opened a new pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a change in pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
aherbert commented on a change in pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99#discussion_r654896924
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
##########
@@ -64,8 +64,8 @@ private FactorialDouble(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- factorialsDouble[i] = cache[i];
+ if (endCopy - 2 >= 0) {
Review comment:
There are now null pointer fails in the travis build. You cannot assume `endCopy - 2 >= 0`. Try changing this to `endCopy - beginCopy >= 0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] arturobernalg commented on a change in pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99#discussion_r654905163
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
##########
@@ -64,8 +64,8 @@ private FactorialDouble(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- factorialsDouble[i] = cache[i];
+ if (endCopy - 2 >= 0) {
Review comment:
I think same result
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
aherbert commented on pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99#issuecomment-864525537
If the array is null it is not the same result. Check the logic a few lines
above. Please run the default maven goal before opening PRs so you can fix
the build errors.
On Sun, 20 Jun 2021, 10:21 Arturo Bernal, ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
> <https://github.com/apache/commons-numbers/pull/99#discussion_r654905163>:
>
> > @@ -64,8 +64,8 @@ private FactorialDouble(int numValues,
> }
>
> // Copy available values.
> - for (int i = beginCopy; i < endCopy; i++) {
> - factorialsDouble[i] = cache[i];
> + if (endCopy - 2 >= 0) {
>
> I think same result
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/commons-numbers/pull/99#discussion_r654905163>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAGYMPWDOU7AAQZJC2FDDJ3TTWXINANCNFSM467YOTEQ>
> .
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] arturobernalg closed pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
arturobernalg closed pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[Numbers][All] Code readability vs System.arraycopy (Was: [...] pull
request #99: [...])
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
FTR: I don't agree with such changes when they obfuscate the code
for no measurable gain (until proven otherwise[1]).
If the class is used properly, the "manual" copying is a minute part of
usage duration; if not used properly (e.g. calling "create()" instead of
"withCache()" on an existing instance), the "saved" copying time will
be marginal wrt the lost time due to repeating the other (likely much
lengthier) computations.
Regards,
Gilles
[1] Through JMH benchmarks _on the use-case at hand_.
Le dim. 20 juin 2021 à 09:35, GitBox <gi...@apache.org> a écrit :
>
>
> arturobernalg commented on a change in pull request #99:
> URL: https://github.com/apache/commons-numbers/pull/99#discussion_r654891918
>
>
>
> ##########
> File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/LogFactorial.java
> ##########
> @@ -62,9 +62,7 @@ private LogFactorial(int numValues,
> }
>
> // Copy available values.
> - for (int i = beginCopy; i < endCopy; i++) {
> - logFactorials[i] = cache[i];
> - }
> + if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, logFactorials, beginCopy, endCopy - 2);
>
> Review comment:
> True. Changed. TY
>
> ##########
> File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
> ##########
> @@ -64,9 +64,7 @@ private FactorialDouble(int numValues,
> }
>
> // Copy available values.
> - for (int i = beginCopy; i < endCopy; i++) {
> - factorialsDouble[i] = cache[i];
> - }
> + if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, factorialsDouble, beginCopy, endCopy - 2);
>
> Review comment:
> True. Changed. TY
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] [commons-numbers] arturobernalg commented on a change in pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99#discussion_r654891918
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/LogFactorial.java
##########
@@ -62,9 +62,7 @@ private LogFactorial(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- logFactorials[i] = cache[i];
- }
+ if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, logFactorials, beginCopy, endCopy - 2);
Review comment:
True. Changed. TY
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
##########
@@ -64,9 +64,7 @@ private FactorialDouble(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- factorialsDouble[i] = cache[i];
- }
+ if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, factorialsDouble, beginCopy, endCopy - 2);
Review comment:
True. Changed. TY
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-numbers] aherbert commented on a change in pull request #99: Replace manual copying of array contents by calls to System.arraycopy().
Posted by GitBox <gi...@apache.org>.
aherbert commented on a change in pull request #99:
URL: https://github.com/apache/commons-numbers/pull/99#discussion_r654887776
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/FactorialDouble.java
##########
@@ -64,9 +64,7 @@ private FactorialDouble(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- factorialsDouble[i] = cache[i];
- }
+ if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, factorialsDouble, beginCopy, endCopy - 2);
Review comment:
This will fail checkstyle. The logic seems fine.
##########
File path: commons-numbers-combinatorics/src/main/java/org/apache/commons/numbers/combinatorics/LogFactorial.java
##########
@@ -62,9 +62,7 @@ private LogFactorial(int numValues,
}
// Copy available values.
- for (int i = beginCopy; i < endCopy; i++) {
- logFactorials[i] = cache[i];
- }
+ if (endCopy - 2 >= 0) System.arraycopy(cache, beginCopy, logFactorials, beginCopy, endCopy - 2);
Review comment:
This will fail checkstyle. The logic seems fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org