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 or why it 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 for 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:
[email protected]
