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/ webap
Am Montag, den 06.12.2010, 22:01 +0100 schrieb Sylvain Laurent:
> >
> >> +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
> :-(
It is "Line Wrapping->Expressions->Binary expressions" [x] Wrap before
operator.
hth
Felix
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
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/ webap
On 06/12/2010 21:01, Sylvain Laurent wrote:
> I fixed most of your points except one, see below.
>>> +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.
You can still drop the casts though and just call
child.removeLifecycleListener() and child.removeContainerListener()
Some of the other fixes still look a little odd. I suspect the Eclipse
formatter. I'll highlight those in a separate e-mail.
Mark
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
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/ webap
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
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/ webap
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.
ASF policy is to strongly discourage author tags.
Your svn client should be configured to set svn:eol-style native on new
text files.
I did wonder about the fixing over time nature of this approach vs. the
immediate fixing of the previous approach and whether or not we should
give users the option of both. On reflection I think providing both will
add a fair amount of complexity and given that this is trying to fix
what is essentially an application bug then I think just having one
approach is fine and this is a better approach than the previous
implementation.
Specific comments in-line.
> 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
> 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.
> +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.
> +} 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.
> +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);
Does the fact the Container implements Lifecycle simplify any of the
other code here?
> 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.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
