Added java 9+ compatibility (bugfix & addition) #139
Conversation
- SignatureChecker: upgraded from ASM5 to ASM7 - ClassFileVisitor: added ability to process Paths (nedded for "jrt:/modules") - SignatureBuilder: added java version switch (<=8 / >=9) and implemented signature creation for java 9+
Update to origin
Inner classes need to be processed first. By using a TreeSet, the files are automatically sorted with inner classes first. eg. like "Outer$Inner.class" before "Outer.class"
Finomosec
changed the title
There was a problem hiding this comment.
Maybe it would be more efficient to directly process the files in postVisitDirectory(...) and create the files set in preVisitDirectory(...). Otherwise when there are a lot of classes to process the files set would become rather large, though it might not matter that much because insertion cost is log(n).
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java
Outdated
Show resolved
Hide resolved
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Main.java
Outdated
Show resolved
Hide resolved
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java
Outdated
Show resolved
Hide resolved
…:/..." (with forward slash)
I fixed this as suggested, as well as the other points. |
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java
Show resolved
Hide resolved
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java
Outdated
Show resolved
Hide resolved
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Main.java
Outdated
Show resolved
Hide resolved
animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Main.java
Outdated
Show resolved
Hide resolved
# Conflicts: # animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/ClassFileVisitor.java # animal-sniffer/src/main/java/org/codehaus/mojo/animal_sniffer/Main.java
|
This PR appears to reference #131 which per #172 (comment) might also fix #172 which in turn is blocking Mockito from upgrading our version: mockito/mockito#2470 Can we maybe revive this PR and land the fix? That would be greatly appreciated, thanks for the work on this plugin! |
|
From my side all looks good. Can you please merge it? Marcono1234 |
I am not a member of this project. Sorry for the confusion in case my review on this pull request made it look otherwise. Hopefully my review was still helpful and not too intrusive. To get this merged you probably have to address @olamy or @slachiewicz.
@TimvdLippe, I think this PR here is unrelated to #131 (unless I am overlooking something); maybe xvik/gradle-animalsniffer-plugin#25 (comment) caused some confusion because it referenced both issues. |
|
@olamy @slachiewicz can you please merge this PR? |
|
Could you help me, please squash to one commit, I'll review after weekend |
| throw exc; | ||
| } | ||
| for (final Path file : files) { | ||
| try (final InputStream inputStream = Files.newInputStream(file)) { |
There was a problem hiding this comment.
nit.: why final? not sure if it helps anything here
There was a problem hiding this comment.
Not intentional. Not harmful. It does not change the behavior here.
In my company we decided to make anything final, that can be final.
It's supposed to make the semantics of the code clearer when reading code.
Remove the finals if you want.
p.s. thanks for the merge.
|
@olamy do you mind publishing a release with this bugfix? |
This PR fixes:
jrt:/modulesinorg.codehaus.mojo.animal_sniffer.Main