You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Konstantin Kolinko <kn...@gmail.com> on 2012/04/12 15:39:13 UTC
Re: svn commit: r1325196 - in /tomcat/maven-plugin/trunk: (...)
2012/4/12 <ol...@apache.org>:
> Author: olamy
> Date: Thu Apr 12 10:32:12 2012
> New Revision: 1325196
>
> URL: http://svn.apache.org/viewvc?rev=1325196&view=rev
> Log:
> fix field name typo
>
> Modified:
> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>
> Modified: tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
> URL: http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
> ==============================================================================
> --- tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java (original)
> +++ tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java Thu Apr 12 10:32:12 2012
> @@ -343,7 +343,7 @@ public abstract class AbstractRunMojo
> */
> private ClassRealm tomcatRealm;
>
> - private ClassLoader originalClassLoaser;
> + private ClassLoader originalClassLoader;
>
> /**
> * The static context
> @@ -383,13 +383,13 @@ public abstract class AbstractRunMojo
> getLog().info( messagesProvider.getMessage( "AbstractRunMojo.nonWar" ) );
> return;
> }
> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
> + originalClassLoader = Thread.currentThread().getContextClassLoader();
> try
> {
>
> if ( useSeparateTomcatClassLoader )
> {
> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
> + originalClassLoader = Thread.currentThread().getContextClassLoader();
> }
> getLog().info( messagesProvider.getMessage( "AbstractRunMojo.runningWar", getWebappUrl() ) );
>
> @@ -413,7 +413,7 @@ public abstract class AbstractRunMojo
> {
> if ( useSeparateTomcatClassLoader )
> {
> - Thread.currentThread().setContextClassLoader( originalClassLoaser );
> + Thread.currentThread().setContextClassLoader( originalClassLoader );
> }
> }
> }
>
> Modified: tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
> URL: http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
> ==============================================================================
> --- tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java (original)
> +++ tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java Thu Apr 12 10:32:12 2012
> @@ -363,7 +363,7 @@ public abstract class AbstractRunMojo
> */
> private ClassRealm tomcatRealm;
>
> - private ClassLoader originalClassLoaser;
> + private ClassLoader originalClassLoader;
>
> // ----------------------------------------------------------------------
> // Mojo Implementation
> @@ -388,7 +388,7 @@ public abstract class AbstractRunMojo
> }
> if ( useSeparateTomcatClassLoader )
> {
> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
> + originalClassLoader = Thread.currentThread().getContextClassLoader();
> }
> try
> {
> @@ -418,7 +418,7 @@ public abstract class AbstractRunMojo
> {
> if ( useSeparateTomcatClassLoader )
> {
> - Thread.currentThread().setContextClassLoader( originalClassLoaser );
> + Thread.currentThread().setContextClassLoader( originalClassLoader );
> }
> }
> }
1. Do you need to keep "originalClassLoader" as a class field?
As far as I see its use is scoped to the execute() method, so that it
can be a local variable there.
2. Tomcat 6 and Tomcat 7 parts of this commit do not match each other.
I mean - see where the value to originalClassLoader is assigned.
There is "if" missing in Tomcat 6 and the second invocation inside of
"try{}" was not removed.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1325196 - in /tomcat/maven-plugin/trunk: (...)
Posted by Olivier Lamy <ol...@apache.org>.
Just fixed that
Thanks for the review!
2012/4/12 Konstantin Kolinko <kn...@gmail.com>:
> 2012/4/12 <ol...@apache.org>:
>> Author: olamy
>> Date: Thu Apr 12 10:32:12 2012
>> New Revision: 1325196
>>
>> URL: http://svn.apache.org/viewvc?rev=1325196&view=rev
>> Log:
>> fix field name typo
>>
>> Modified:
>> tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>> tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>>
>> Modified: tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java
>> URL: http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
>> ==============================================================================
>> --- tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java (original)
>> +++ tomcat/maven-plugin/trunk/tomcat6-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat6/AbstractRunMojo.java Thu Apr 12 10:32:12 2012
>> @@ -343,7 +343,7 @@ public abstract class AbstractRunMojo
>> */
>> private ClassRealm tomcatRealm;
>>
>> - private ClassLoader originalClassLoaser;
>> + private ClassLoader originalClassLoader;
>>
>> /**
>> * The static context
>> @@ -383,13 +383,13 @@ public abstract class AbstractRunMojo
>> getLog().info( messagesProvider.getMessage( "AbstractRunMojo.nonWar" ) );
>> return;
>> }
>> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
>> + originalClassLoader = Thread.currentThread().getContextClassLoader();
>> try
>> {
>>
>> if ( useSeparateTomcatClassLoader )
>> {
>> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
>> + originalClassLoader = Thread.currentThread().getContextClassLoader();
>> }
>> getLog().info( messagesProvider.getMessage( "AbstractRunMojo.runningWar", getWebappUrl() ) );
>>
>> @@ -413,7 +413,7 @@ public abstract class AbstractRunMojo
>> {
>> if ( useSeparateTomcatClassLoader )
>> {
>> - Thread.currentThread().setContextClassLoader( originalClassLoaser );
>> + Thread.currentThread().setContextClassLoader( originalClassLoader );
>> }
>> }
>> }
>>
>> Modified: tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java
>> URL: http://svn.apache.org/viewvc/tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java?rev=1325196&r1=1325195&r2=1325196&view=diff
>> ==============================================================================
>> --- tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java (original)
>> +++ tomcat/maven-plugin/trunk/tomcat7-maven-plugin/src/main/java/org/apache/tomcat/maven/plugin/tomcat7/run/AbstractRunMojo.java Thu Apr 12 10:32:12 2012
>> @@ -363,7 +363,7 @@ public abstract class AbstractRunMojo
>> */
>> private ClassRealm tomcatRealm;
>>
>> - private ClassLoader originalClassLoaser;
>> + private ClassLoader originalClassLoader;
>>
>> // ----------------------------------------------------------------------
>> // Mojo Implementation
>> @@ -388,7 +388,7 @@ public abstract class AbstractRunMojo
>> }
>> if ( useSeparateTomcatClassLoader )
>> {
>> - originalClassLoaser = Thread.currentThread().getContextClassLoader();
>> + originalClassLoader = Thread.currentThread().getContextClassLoader();
>> }
>> try
>> {
>> @@ -418,7 +418,7 @@ public abstract class AbstractRunMojo
>> {
>> if ( useSeparateTomcatClassLoader )
>> {
>> - Thread.currentThread().setContextClassLoader( originalClassLoaser );
>> + Thread.currentThread().setContextClassLoader( originalClassLoader );
>> }
>> }
>> }
>
> 1. Do you need to keep "originalClassLoader" as a class field?
>
> As far as I see its use is scoped to the execute() method, so that it
> can be a local variable there.
>
>
> 2. Tomcat 6 and Tomcat 7 parts of this commit do not match each other.
> I mean - see where the value to originalClassLoader is assigned.
>
> There is "if" missing in Tomcat 6 and the second invocation inside of
> "try{}" was not removed.
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
--
Olivier Lamy
Talend: http://coders.talend.com
http://twitter.com/olamy | http://linkedin.com/in/olamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org