tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: svn commit: r948294 - in /tomcat/tc5.5.x/trunk: ./ container/webapps/docs/ jasper/src/share/org/apache/jasper/ jasper/src/share/org/apache/jasper/compiler/ jasper/src/share/org/apache/jasper/security/ jasper/src/share/org/apache/jasper/servlet/
Date Wed, 26 May 2010 16:52:37 GMT
2010/5/26 sebb <sebbaz@gmail.com>:
>> The "removed" variable is only checked whether it is zero or some
>>  positive value. So, the actual value of the variable before the
>>  increment operation is irrelevant.  It could be written as "removed =
>>  1;" and assignment is atomic.
>
> In that case, why not do so - or make it a boolean?
>

If you see the old code of "isRemoved()" -- before this patch -- it
used to check "if (removed > 1)". So, "0", "1", and ">1" were distinct
states.

I agree with you that this private field can now be made boolean.

>>  Are there any observable consequences? If they are, you may file an issue.
>
> A brief scan of the code suggests not, unless it is possible for the
> counter to overflow.
>
> But it is confusing to have a method called incrementRemoved() and a
> corresponding counter which is actually being used purely as a flag.
>

0 -> 1 is an increment, as well as false -> true. ;)

How the counter is implemented should not bother one who calls this
method. I am fine with its name. (I have thought of a pair of
alternatives, but all they are more confusing than the current name).

> May I suggest adding a comment to document this for future maintainers?
>

;)

Best regards,
Konstantin Kolinko

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


Mime
View raw message