Fix new Error Prone 2.4.0 detected issues. #403
Conversation
| @@ -279,8 +278,7 @@ public static void annotateBytecodeInJar( | |||
| // Do not use JarInputStream in place of JarFile/JarEntry. JarInputStream misses MANIFEST.MF | |||
| // while iterating over the entries in the stream. | |||
| // Reference: https://bugs.openjdk.java.net/browse/JDK-8215788 | |||
| for (Enumeration<JarEntry> entries = inputJar.entries(); entries.hasMoreElements(); ) { | |||
| JarEntry jarEntry = entries.nextElement(); | |||
| for (JarEntry jarEntry : inputJar.stream().collect(ImmutableList.toImmutableList())) { | |||
There was a problem hiding this comment.
In these cases can we do a forEach() on the stream rather than collecting in a list and looping over that? That seems a bit cleaner and avoids copying to another list.
There was a problem hiding this comment.
No, we can't. The code bellow can throw IOException. If that happens within a lambda, we need to catch it there, if it happens within an iteration we can propagate it up.
There was a problem hiding this comment.
Maybe add a code comment to that effect? Or not if you think it's obvious, up to you
There was a problem hiding this comment.
Also done 😄 (before I saw your comment, but not sure if before you made it...)
| for (Enumeration<JarEntry> entries = jar1.entries(); entries.hasMoreElements(); ) { | ||
| jar1Entries.add(entries.nextElement().getName()); | ||
| } | ||
| jar1.stream().forEach(entry -> jar1Entries.add(entry.getName())); |
There was a problem hiding this comment.
Here I would do something like Set<String> jar1Entries = jar1.stream().map(Entry::getName).collect(Collectors::toSet). Not sure of exact syntax but something like that. Same for the other similar cases.
| JarEntry jarEntry = entries.nextElement(); | ||
| // Note: we can't just put the code below inside stream().forach(), because it can throw | ||
| // IOException. | ||
| for (JarEntry jarEntry : inputJar.stream().collect(ImmutableList.toImmutableList())) { |
There was a problem hiding this comment.
Here and below: the list collection (= copy) can be avoided e.g. as follows:
| for (JarEntry jarEntry : inputJar.stream().collect(ImmutableList.toImmutableList())) { | |
| for (JarEntry jarEntry : (Iterable<JarEntry>) inputJar.stream()::iterator) { |
This could be relevant when dealing with very large JAR files.
(Due to generics this trick doesn't seem to quite work for the ZipFile loops 🤔.)
There was a problem hiding this comment.
Ah very cool! I didn't know you could convert streams to iterators (and iterables) so easily!
There was a problem hiding this comment.
(Just so you know: aware of this note and planning to do a follow up PR, but I haven't had a chance :) Unless someone else wants to go for it, I'll try to follow up early next week)
See #403 discussion, credit for this optimization goes to @Stephan202. Note the comment edit for the AAR case. ZipFile is just surprisingly hostile to copy-free for-each iteration.
See #403 discussion, credit for this optimization goes to @Stephan202. Note the comment edit for the AAR case. ZipFile is just surprisingly hostile to copy-free for-each iteration.
This removes the suppressions introduced in #400 and makes more permanent fixes.
Fixes #402.