kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637354880
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources,
JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done
concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the
value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
I made the `ThreadLocal` final, as requested. I decided not to refactor
to create more extra ASM visitor instances just so as to avoid the thread-local
field. I do not see that is should "blow up anyone's face", as @rmannibucau
said. The code is simple enough and nothing I am doing there is shady or rocket
science. Of course, you can always solve a problem by introducing another layer
of complexity, but I have decided against it here and not to cater to someone's
personal antipathy for thread-locals. Sorry, but this is my decision for this
contribution. We wasted too much time fior discussing this tiny detail already.
I would rather have gotten feedback concerning the functionality as such.
@rmannibucau, @eolivelli, would you please also review #88. I have been
waiting for a review and for the final merge for too long already. I basically
added a new feature for source code shading respecting package exclusions,
hence fixing the existing, fragmentary functionality, which was completely
broken IMO. Now it is slower, but it does what it should.
--
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:
us...@infra.apache.org
