You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "RestartCoding (via GitHub)" <gi...@apache.org> on 2023/06/12 10:02:42 UTC
[GitHub] [commons-lang] RestartCoding opened a new pull request, #1065: fix: Method description is not accurate
RestartCoding opened a new pull request, #1065:
URL: https://github.com/apache/commons-lang/pull/1065
ArrayUtils.subarray(final boolean[] array, int startIndexInclusive, int endIndexExclusive)
--
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
Re: [PR] Some ArrayUtils method descriptions are not accurate [commons-lang]
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-2030710084
I would close it. The javadoc currently reflects what the code is doing. If you read the javadoc as is you should get the understanding that the method is _trying_ to protect you from a mess up for the range of the index arguments. However it does not stop real stupidity. You can do this and create a huge array via negative overflow:
```java
int[] a = {0};
// Tries to create an array of size Integer.MAX_VALUE !
subarray(a, 1, Integer.MIN_VALUE);
```
A caller may soon find out this is a bad call due to either memory allocation error, or an error from the subsequent arraycopy which could fail as the source array is too small. If the source array happens to be huge as well then you could end up copying something. But currently this method does not protect from such bad arguments.
This method is from lang 2.1 (circa Nov 2005). Arrays.copyOfRange is in JDK 1.6 (circa Dec 2006) and does the copy correctly if you specify valid indices. Since this is doing checks to make it null safe and index safe we wish to avoid delegating to copyOfRange after we have clipped indices to avoid duplicating index checks; we also wish to return a singleton empty array if possible. A fix would be to remove the check on the size and do:
```java
if (endIndexExclusive <= startIndexInclusive) {
return // empty array
}
// Now compute size...
```
I think this would maintain the intention of the method. To allow you to call with basically any argument and get either a null array, empty array or a populated (sub-)array if you happen to have covered some/all of the range of the input array with the arguments. If you remove this intention, then you may as well just deprecate in favour of copyOfRange.
--
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
[GitHub] [commons-lang] aherbert commented on pull request #1065: fix: Method description is not accurate
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1589107906
The change applied to the `subArray(boolean[], ...)` also applies to 7 other `subArray` methods in the same class. Can you update to fix them all. Thanks.
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1065: Some ArrayUtils method descriptions are not accurate
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1677230914
Ping?
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1065: fix: Method description is not accurate
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1611493642
@aherbert Does this look OK to you now?
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1065: fix: Method description is not accurate
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1591087674
I agree that only the Javadoc should be updated.
--
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
Re: [PR] Some ArrayUtils method descriptions are not accurate [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory closed pull request #1065: Some ArrayUtils method descriptions are not accurate
URL: https://github.com/apache/commons-lang/pull/1065
--
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
[GitHub] [commons-lang] RestartCoding commented on pull request #1065: fix: Method description is not accurate
Posted by "RestartCoding (via GitHub)" <gi...@apache.org>.
RestartCoding commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1590280142
I think it is fine to just change method description to make it precise. It's not necessary to change method itself. and what do you say?
--
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
Re: [PR] Some ArrayUtils method descriptions are not accurate [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1951329751
@RestartCoding
ping
--
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
[GitHub] [commons-lang] aherbert commented on pull request #1065: fix: Method description is not accurate
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1590949397
No need to change the method. But I suggested changing the description of the `startIndexInclusive` too since the empty array result is for `>=` not `>`:
```Java
/**
* ...
* @param startIndexInclusive the starting index. Undervalue (<0)
* is promoted to 0, overvalue (>= array.length) results
* in an empty array.
* @param endIndexExclusive elements up to endIndex-1 are present in the
* returned subarray. Undervalue (<= startIndex) results in an
* empty array, overvalue (> array.length) is demoted to
* array length.
* ...
*/
```
--
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
[GitHub] [commons-lang] RestartCoding commented on pull request #1065: fix: Method description is not accurate
Posted by "RestartCoding (via GitHub)" <gi...@apache.org>.
RestartCoding commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1589129910
Ok，I'll try it tomorrow.
---- Replied Message ----
| From | Alex ***@***.***> |
| Date | 06/13/2023 19:24 |
| To | ***@***.***> |
| Cc | ***@***.***>***@***.***> |
| Subject | Re: [apache/commons-lang] fix: Method description is not accurate (PR #1065) |
The change applied to the subArray(boolean[], ...) also applies to 7 other subArray methods in the same class. Can you update to fix them all. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
--
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
Re: [PR] Some ArrayUtils method descriptions are not accurate [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-2030599744
@aherbert Thoughts on closing this PR or merging it despite the shortcomings?
--
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
[GitHub] [commons-lang] aherbert commented on pull request #1065: fix: Method description is not accurate
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1589128201
Note that the javadoc mixes the use of what happens when a parameter is 'undervalue' (too small) and 'overvalue' (too large), with the effect on the parameter (undervalue set to 0 for start index; overvalue set to array.length for end index), or the effect on the result (overvalue produces an empty array for start index; undervalue produces an empty array for end index). It is a bit strange.
However for the description of what produces an empty array then if we change to `Undervalue (<= startIndex) produces empty array` we should also change to `overvalue (>= array.length) results in an empty array`. The two should also consolidate on either `results in an empty array` or `produces (an) empty array`.
--
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
[GitHub] [commons-lang] RestartCoding commented on pull request #1065: fix: Method description is not accurate
Posted by "RestartCoding (via GitHub)" <gi...@apache.org>.
RestartCoding commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1590282899
I think it's fine to change method description to make it precise. It's not necessary to change method itself and what do you say?
--
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
[GitHub] [commons-lang] garydgregory commented on pull request #1065: Some ArrayUtils method descriptions are not accurate
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-1646926950
@RestartCoding
This does not seem complete in the sense that the "overvalue" comments @aherbert mentions are not addressed.
--
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
Re: [PR] Some ArrayUtils method descriptions are not accurate [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1065:
URL: https://github.com/apache/commons-lang/pull/1065#issuecomment-2033092795
Closing on consensus.
--
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