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, slaur...@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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
