tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Laurent <sylvain.laur...@m4x.org>
Subject Re: svn commit: r1042482 - in /tomcat/trunk: ./ conf/ java/org/apache/catalina/core/ java/org/apache/catalina/loader/ java/org/apache/tomcat/util/threads/ res/confinstall/ webapps/ webapps/docs/ webapps/docs/config/
Date Mon, 06 Dec 2010 21:01:29 GMT
I fixed most of your points except one, see below.

On 6 déc. 2010, at 01:09, Mark Thomas wrote:

> On 05/12/2010 22:54, slaurent@apache.org wrote:
>> Author: slaurent
>> Date: Sun Dec  5 22:54:05 2010
>> New Revision: 1042482
>> 
>> URL: http://svn.apache.org/viewvc?rev=1042482&view=rev
> 
> General comments:
> 
> Numerous new lines with length > 80 chars.

Actually I had configured eclipse for 80 chars but the default line-wrapping settings are
such that it does not wrap in several cases...

> ASF policy is to strongly discourage author tags.
fixed

> Your svn client should be configured to set svn:eol-style native on new
> text files.
OK, done for future new files.

>> Modified:
>>    tomcat/trunk/   (props changed)
>>    tomcat/trunk/conf/   (props changed)
>>    tomcat/trunk/webapps/    (props changed)
> -1 to all these changes.
> a) This is not a Tomcat instance for running. That is created in output.
> b) It reverts a useful change I made earlier today
Sorry, I did not realize there could be changes on directories with SVN. For debugging I launch
tomcat from the base directory and this creates directories likes log, work, etc...
I believe you fixed this.

>> Added: tomcat/trunk/java/org/apache/catalina/core/ThreadLocalLeakPreventionListener.java
> 
>> +    private static final Log log = LogFactory
>> +            .getLog(ThreadLocalLeakPreventionListener.class);
> That is an odd place for a line-break that I find hard to read. I find
> either of the following easier to read:
> 
> private static final Log log =
>        LogFactory.getLog(ThreadLocalLeakPreventionListener.class);
> 
> private static final Log log = LogFactory.getLog(
>        ThreadLocalLeakPreventionListener.class);
> 
> There are multiple instances of this throughout the commit.

Still the default eclipse formatter... We should really provide eclipse formatter settings.
Formatting code by hand is really a waste of time.

> 
>> +    public void lifecycleEvent(LifecycleEvent event) {
>> +        try {
>> +            Lifecycle lifecycle = event.getLifecycle();
>> +            if (Lifecycle.AFTER_START_EVENT.equals(event.getType())
>> +                    && lifecycle instanceof Server) {
> With the operator on the new line it is easy to miss what is going on.
> Generally, Tomcat style is to put the operator on the first line. e.g.
> if (Lifecycle.AFTER_START_EVENT.equals(event.getType()) &&
>        lifecycle instanceof Server) {
> 
> There are multiple instances of this throughout the commit.

Still eclipse. But I did not manage to find a setting to change this behavior :-(

> 
>> +        } catch (Exception e) {
>> +            log.error("Exception processing event " + event, e);
>> +        }
> Error messages really should be using a StringManager.
> There are multiple instances of this throughout the commit.

done.

>> +    protected void processContainerRemoveChild(Container parent, Container child)
{
>> +
>> +        if (log.isDebugEnabled())
>> +            log.debug("Process removeChild[parent=" + parent + ",child="
>> +                    + child + "]");
>> +
>> +        try {
>> +            if (child instanceof Context) {
>> +                Context context = (Context) child;
>> +                context.removeLifecycleListener(this);
>> +            } else if (child instanceof Host) {
>> +                Host host = (Host) child;
>> +                host.removeContainerListener(this);
>> +            } else if (child instanceof Engine) {
>> +                Engine engine = (Engine) child;
>> +                engine.removeContainerListener(this);
>> +            }
> Why all this? Just use:
> child.removeLifecycleListener(this);

No, it's not the same. The listener is interested in both LifeCycle events for Context, and
Container events for Host and Engine. These are not the same things.

> Does the fact the Container implements Lifecycle simplify any of the
> other code here?
Can't tell.

>> Modified: tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoader.java
>> -    private void clearThreadLocalMap(Object map, Field internalTableField)
>> +    private void checkThreadLocalMapForLeaks(Object map, Field internalTableField)
>>             throws NoSuchMethodException, IllegalAccessException,
>>             NoSuchFieldException, InvocationTargetException {
> Not all of those Exceptions are required after this commit.
Fixed.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message