Conversation
| import org.apache.commons.collections4.collection.TransformedCollection; | ||
| import org.apache.commons.collections4.collection.UnmodifiableBoundedCollection; | ||
| import org.apache.commons.collections4.collection.UnmodifiableCollection; | ||
| import org.apache.commons.collections4.collection.*; |
There was a problem hiding this comment.
It is preferable to leave import each class individually.
| @SuppressWarnings("rawtypes") // we deliberately use the raw type here | ||
| public static final Collection EMPTY_COLLECTION = | ||
| UnmodifiableCollection.unmodifiableCollection(new ArrayList<Object>()); | ||
| public static final Collection EMPTY_COLLECTION = Collections.emptyList(); |
| * @return a boolean indicating whether the collection has changed or not. | ||
| * @throws NullPointerException if the collection or iterator is null | ||
| */ | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Is that necessary? I'm comparing with a local working copy of the trunk branch, looking ad the addAll method I have, it doesn't seem necessary. Perhaps it needs to be rebased and revised against last changes?
There was a problem hiding this comment.
Of cource, it is minor. this is not necessary, but if it is applied, warnings about not correct cast will disappear. I talked about this code: collection.addAll((Collection<? extends C>) iterable)
There was a problem hiding this comment.
I might still be missing something here... Reviewing online, it didn't seem to be necessary. Just imported the code to Eclipse, and the project has 41 Warnings. This annotation being one of the warnings, being marked as "Unnecessary @SuppressWarnings("unchecked")".
There was a problem hiding this comment.
Hmm, It is very interesting. My IDE, (i use intellij idea), warns me about unchecked cast. Maybe, it is a false warning. if you think that is not necessary, i will not persist :)
There was a problem hiding this comment.
Removing in NetBeans (how nostalgic it was to use it) also did not raise any warnings. However, NetBeans has a lot of other warnings and errors that do not appear in Eclipse. I thought the IDE's would all agree upon what to check or not, but searching the Internet I found some other reports from differences in Eclipse/IntelliJ regarding these suppresswarnings cases. Let's leave as is for now, and we can revisit it later. So all good to be merged I believe?
There was a problem hiding this comment.
I agree with you. You are right. I had try to compile the code by javac with flag -Xlint:all and did not see any warnings. Then Intellij Idea shows false warning or the warning are not for compiler, it is more for a programmer.
Yes, all good to be merged.
P.S. Netbeans is in my heart, too :)
| } else if (object instanceof Iterable<?>) { | ||
| final Iterable<?> iterable = (Iterable<?>) object; | ||
| return IterableUtils.get(iterable, i); | ||
| } else if (object instanceof Collection<?>) { |
There was a problem hiding this comment.
Is this one really safe to remove? It seems to assume that the else condition will handle with the same behaviour, but I have not tested it to confirm.
Might be worth also moving that if (object == null) check to the top.
There was a problem hiding this comment.
Removing of this condition is safe. Moreover, this check is useless, because, any collection implements interface Iterable, and if object is Collection, then previous condition (object instanceof Iterable<?>) is true.
There was a problem hiding this comment.
Nice. Will check out the code to review with the IDE, plus some coffee 💤 I think it looks good to be merged. Thanks for the quick replies @izebit
There was a problem hiding this comment.
Oooohh, just re-read, and finally understood. Sorry, nice catch again.
kinow
left a comment
There was a problem hiding this comment.
All good, except one SupressWarnings that seems unnecessary?
| * @return a boolean indicating whether the collection has changed or not. | ||
| * @throws NullPointerException if the collection or iterator is null | ||
| */ | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
I might still be missing something here... Reviewing online, it didn't seem to be necessary. Just imported the code to Eclipse, and the project has 41 Warnings. This annotation being one of the warnings, being marked as "Unnecessary @SuppressWarnings("unchecked")".
| } else if (object instanceof Iterable<?>) { | ||
| final Iterable<?> iterable = (Iterable<?>) object; | ||
| return IterableUtils.get(iterable, i); | ||
| } else if (object instanceof Collection<?>) { |
There was a problem hiding this comment.
Oooohh, just re-read, and finally understood. Sorry, nice catch again.
1 similar comment
|
Placeholder to keep track of the change https://issues.apache.org/jira/browse/COLLECTIONS-603 |
|
Merged. Issue created/updated. changes.xml updated. All tests pass, site reports look good, nothing for CollectionUtils in PMD, CPD, FindBugs. No changes in methods signatures. Thanks for your contribution @izebit ! |
No description provided.