You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2011/06/28 02:08:55 UTC
svn commit: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt
tc6.0.x/trunk/STATUS.txt
Author: kkolinko
Date: Tue Jun 28 00:08:55 2011
New Revision: 1140383
URL: http://svn.apache.org/viewvc?rev=1140383&view=rev
Log:
veto
Modified:
tomcat/tc5.5.x/trunk/STATUS.txt
tomcat/tc6.0.x/trunk/STATUS.txt
Modified: tomcat/tc5.5.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc5.5.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
==============================================================================
--- tomcat/tc5.5.x/trunk/STATUS.txt (original)
+++ tomcat/tc5.5.x/trunk/STATUS.txt Tue Jun 28 00:08:55 2011
@@ -88,7 +88,8 @@ PATCHES PROPOSED TO BACKPORT:
http://svn.apache.org/viewvc?rev=1138950&view=rev
http://svn.apache.org/viewvc?rev=1138953&view=rev
+1: markt
- -1:
+ -1: kkolinko: because of magling the '_' character, which will break existing
+ code - see my comment in STATUS file of TC 6.0.
* Multiple improvements to the Windows Installer
- https://issues.apache.org/bugzilla/show_bug.cgi?id=33262
Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Tue Jun 28 00:08:55 2011
@@ -169,7 +169,14 @@ PATCHES PROPOSED TO BACKPORT:
http://svn.apache.org/viewvc?rev=1138950&view=rev
http://svn.apache.org/viewvc?rev=1138953&view=rev
+1: markt, schultz
- -1:
+ -1: kkolinko: 1) It would be OK if it were mangling only reserved words
+ (where the tag wouldn't compile previously, so there were no regressions),
+ but JspUtils.makeJavaIdentifier() mangles '_' character, which will break
+ code that was previously working. E.g. <%=hello_world%>
+ 2) Maybe it would be better to prefix the names with [_]jsp_ and use some
+ numbered suffix, to never collide with names that people may use in their
+ code.
+
* Multiple improvements to the Windows Installer
- https://issues.apache.org/bugzilla/show_bug.cgi?id=33262
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 28/06/2011 14:33, Konstantin Kolinko wrote:
> 2011/6/28 Mark Thomas <ma...@apache.org>:
>>
>>> IMHO it is '_' that was treated as special char, because mangleChar()
>>> prepends '_' as well. Otherwise the mangling will be unreversible.
>>
>> Does that matter? Do we need to able to reverse this?
>
> To avoid collisions between mangled names and names that did not need
> to be mangled.
collisions are always going to be possible unless we go down the _jsp*
route which - while spec compliant - would break even more stuff then
than the original hello_world regression we were trying to solve.
>>> The use case of '.' is just to use shorter name for a frequent use case.
>>> E.g. "index.jsp" -> "index_jsp.class".
>>
>> It would be nice to keep that.
>
> Yes. It would be some surprise if names of (pre)compiled jsps will
> change between minor versions.
>
>> I suppose we could add a
>> periodToUnderscore parameter to makeJavaIdentifier() and essnetially
>> have both approaches. The current one for file names, the new one for
>> variable names.
>
> Or better just add a separate method.
I never did like code duplication ;)
I'm close to having a patch ready for this. Just need to run through the
unit tests.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/6/28 Mark Thomas <ma...@apache.org>:
>
>> IMHO it is '_' that was treated as special char, because mangleChar()
>> prepends '_' as well. Otherwise the mangling will be unreversible.
>
> Does that matter? Do we need to able to reverse this?
To avoid collisions between mangled names and names that did not need
to be mangled.
>> The use case of '.' is just to use shorter name for a frequent use case.
>> E.g. "index.jsp" -> "index_jsp.class".
>
> It would be nice to keep that.
Yes. It would be some surprise if names of (pre)compiled jsps will
change between minor versions.
> I suppose we could add a
> periodToUnderscore parameter to makeJavaIdentifier() and essnetially
> have both approaches. The current one for file names, the new one for
> variable names.
Or better just add a separate method.
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: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 28/06/2011 12:21, Konstantin Kolinko wrote:
> 2011/6/28 Mark Thomas <ma...@apache.org>:
>> On 28/06/2011 11:38, Konstantin Kolinko wrote:
>>> 2011/6/28 Mark Thomas <ma...@apache.org>:
>>>
>>> There is one more problem: you cannot introduce new variables that
>>> will be seen by user's code, unless those are prefixed with reserved
>>> prefix of "jsp_" ( "_jsp_") per JSP.9.1.2. That is because someone can
>>> declare a local variable with the same name.
>>
>> JSP.9.1.2 certainly implies that but enforcing that would break the
>> <%=attributeName%> usage. I'm happy not worrying about that for now.
>>
>>>> That said, I take the point regarding regressions. I'm sure lots of
>>>> folks will have used <%=attributeName%> or ${attributeName} as a
>>>> shortcut although I don't currently see anything to support that usage
>>>> in the JSP specification.
>>
>> I was thinking along these lines:
>>
>> Index: java/org/apache/jasper/compiler/JspUtil.java
>> ===================================================================
>> --- java/org/apache/jasper/compiler/JspUtil.java (revision 1140509)
>> +++ java/org/apache/jasper/compiler/JspUtil.java (working copy)
>> @@ -810,10 +810,8 @@
>> }
>> for (int i = 0; i < identifier.length(); i++) {
>> char ch = identifier.charAt(i);
>> - if (Character.isJavaIdentifierPart(ch) && ch != '_') {
>> + if (Character.isJavaIdentifierPart(ch)) {
>> modifiedIdentifier.append(ch);
>> - } else if (ch == '.') {
>> - modifiedIdentifier.append('_');
>> } else {
>> modifiedIdentifier.append(mangleChar(ch));
>> }
>>
>>
>> Essentially, no longer treat '.' as a special case. That removes the
>> needs to escape _ which should prevent any regressions. However, I do
>> wonder why '.' was treated as a special case in the first place. Time to
>> do some testing...
It appears to be OK - at least for the unit tests.
> IMHO it is '_' that was treated as special char, because mangleChar()
> prepends '_' as well. Otherwise the mangling will be unreversible.
Does that matter? Do we need to able to reverse this?
> The use case of '.' is just to use shorter name for a frequent use case.
> E.g. "index.jsp" -> "index_jsp.class".
It would be nice to keep that. I suppose we could add a
periodToUnderscore parameter to makeJavaIdentifier() and essnetially
have both approaches. The current one for file names, the new one for
variable names.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/6/28 Mark Thomas <ma...@apache.org>:
> On 28/06/2011 11:38, Konstantin Kolinko wrote:
>> 2011/6/28 Mark Thomas <ma...@apache.org>:
>>
>> There is one more problem: you cannot introduce new variables that
>> will be seen by user's code, unless those are prefixed with reserved
>> prefix of "jsp_" ( "_jsp_") per JSP.9.1.2. That is because someone can
>> declare a local variable with the same name.
>
> JSP.9.1.2 certainly implies that but enforcing that would break the
> <%=attributeName%> usage. I'm happy not worrying about that for now.
>
>>> That said, I take the point regarding regressions. I'm sure lots of
>>> folks will have used <%=attributeName%> or ${attributeName} as a
>>> shortcut although I don't currently see anything to support that usage
>>> in the JSP specification.
>
> I was thinking along these lines:
>
> Index: java/org/apache/jasper/compiler/JspUtil.java
> ===================================================================
> --- java/org/apache/jasper/compiler/JspUtil.java (revision 1140509)
> +++ java/org/apache/jasper/compiler/JspUtil.java (working copy)
> @@ -810,10 +810,8 @@
> }
> for (int i = 0; i < identifier.length(); i++) {
> char ch = identifier.charAt(i);
> - if (Character.isJavaIdentifierPart(ch) && ch != '_') {
> + if (Character.isJavaIdentifierPart(ch)) {
> modifiedIdentifier.append(ch);
> - } else if (ch == '.') {
> - modifiedIdentifier.append('_');
> } else {
> modifiedIdentifier.append(mangleChar(ch));
> }
>
>
> Essentially, no longer treat '.' as a special case. That removes the
> needs to escape _ which should prevent any regressions. However, I do
> wonder why '.' was treated as a special case in the first place. Time to
> do some testing...
IMHO it is '_' that was treated as special char, because mangleChar()
prepends '_' as well. Otherwise the mangling will be unreversible.
The use case of '.' is just to use shorter name for a frequent use case.
E.g. "index.jsp" -> "index_jsp.class".
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: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 28/06/2011 11:38, Konstantin Kolinko wrote:
> 2011/6/28 Mark Thomas <ma...@apache.org>:
>> On 28/06/2011 01:08, kkolinko@apache.org wrote:
>>> Modified: tomcat/tc6.0.x/trunk/STATUS.txt
>>> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
>>>
>>> http://svn.apache.org/viewvc?rev=1138950&view=rev
>>> http://svn.apache.org/viewvc?rev=1138953&view=rev
>>> +1: markt, schultz
>>> - -1:
>>> + -1: kkolinko: 1) It would be OK if it were mangling only reserved words
>>> + (where the tag wouldn't compile previously, so there were no regressions),
>>> + but JspUtils.makeJavaIdentifier() mangles '_' character, which will break
>>> + code that was previously working. E.g. <%=hello_world%>
>>> + 2) Maybe it would be better to prefix the names with [_]jsp_ and use some
>>> + numbered suffix, to never collide with names that people may use in their
>>> + code.
>>
>> I don't see anything in the JSP specification that says tag file
>> attributes must be exposed as Java variables with the same name. Given
>> that a tag file attribute can have any name valid in XML, that
>> requirement would be impossible to meet since may of those names would
>> be invalid as Java identifiers.
>>
>> There is a requirement to expose attributes (with the same name) as page
>> scoped variables. If I switch the test case to use ${hello_world} rather
>> than <%=hello_world%> it works. However, that causes it's own set of
>> problems when a Java keyword is used as an attribute name due to the
>> restrictions of the EL specification.
>>
>> AFAICT, the only solution that is guaranteed (by the specification) to
>> work for any attribute name in a tag file is:
>> ${pageScope['attributename']}
>> Unless I have missed something in the specification (always a
>> possibility) that anything else works is a fortunate side-effect of the
>> implementation.
>>
>
> Interesting. Though I used this feature a lot, since TC 5.5.
>
> Eclipse IDE used to hilite these usages in red several years ago, but
> nowadays it has support for such usage.
>
>> On this basis, I think the new behaviour is compliant with the
>> specification.
>
> There is one more problem: you cannot introduce new variables that
> will be seen by user's code, unless those are prefixed with reserved
> prefix of "jsp_" ( "_jsp_") per JSP.9.1.2. That is because someone can
> declare a local variable with the same name.
JSP.9.1.2 certainly implies that but enforcing that would break the
<%=attributeName%> usage. I'm happy not worrying about that for now.
>> That said, I take the point regarding regressions. I'm sure lots of
>> folks will have used <%=attributeName%> or ${attributeName} as a
>> shortcut although I don't currently see anything to support that usage
>> in the JSP specification.
I was thinking along these lines:
Index: java/org/apache/jasper/compiler/JspUtil.java
===================================================================
--- java/org/apache/jasper/compiler/JspUtil.java (revision 1140509)
+++ java/org/apache/jasper/compiler/JspUtil.java (working copy)
@@ -810,10 +810,8 @@
}
for (int i = 0; i < identifier.length(); i++) {
char ch = identifier.charAt(i);
- if (Character.isJavaIdentifierPart(ch) && ch != '_') {
+ if (Character.isJavaIdentifierPart(ch)) {
modifiedIdentifier.append(ch);
- } else if (ch == '.') {
- modifiedIdentifier.append('_');
} else {
modifiedIdentifier.append(mangleChar(ch));
}
Essentially, no longer treat '.' as a special case. That removes the
needs to escape _ which should prevent any regressions. However, I do
wonder why '.' was treated as a special case in the first place. Time to
do some testing...
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/6/28 Mark Thomas <ma...@apache.org>:
> On 28/06/2011 01:08, kkolinko@apache.org wrote:
>> Modified: tomcat/tc6.0.x/trunk/STATUS.txt
>> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
>>
>> http://svn.apache.org/viewvc?rev=1138950&view=rev
>> http://svn.apache.org/viewvc?rev=1138953&view=rev
>> +1: markt, schultz
>> - -1:
>> + -1: kkolinko: 1) It would be OK if it were mangling only reserved words
>> + (where the tag wouldn't compile previously, so there were no regressions),
>> + but JspUtils.makeJavaIdentifier() mangles '_' character, which will break
>> + code that was previously working. E.g. <%=hello_world%>
>> + 2) Maybe it would be better to prefix the names with [_]jsp_ and use some
>> + numbered suffix, to never collide with names that people may use in their
>> + code.
>
> I don't see anything in the JSP specification that says tag file
> attributes must be exposed as Java variables with the same name. Given
> that a tag file attribute can have any name valid in XML, that
> requirement would be impossible to meet since may of those names would
> be invalid as Java identifiers.
>
> There is a requirement to expose attributes (with the same name) as page
> scoped variables. If I switch the test case to use ${hello_world} rather
> than <%=hello_world%> it works. However, that causes it's own set of
> problems when a Java keyword is used as an attribute name due to the
> restrictions of the EL specification.
>
> AFAICT, the only solution that is guaranteed (by the specification) to
> work for any attribute name in a tag file is:
> ${pageScope['attributename']}
> Unless I have missed something in the specification (always a
> possibility) that anything else works is a fortunate side-effect of the
> implementation.
>
Interesting. Though I used this feature a lot, since TC 5.5.
Eclipse IDE used to hilite these usages in red several years ago, but
nowadays it has support for such usage.
> On this basis, I think the new behaviour is compliant with the
> specification.
There is one more problem: you cannot introduce new variables that
will be seen by user's code, unless those are prefixed with reserved
prefix of "jsp_" ( "_jsp_") per JSP.9.1.2. That is because someone can
declare a local variable with the same name.
>
> That said, I take the point regarding regressions. I'm sure lots of
> folks will have used <%=attributeName%> or ${attributeName} as a
> shortcut although I don't currently see anything to support that usage
> in the JSP specification.
>
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: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 28/06/2011 01:08, kkolinko@apache.org wrote:
> Modified: tomcat/tc6.0.x/trunk/STATUS.txt
> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
> ==============================================================================
> --- tomcat/tc6.0.x/trunk/STATUS.txt (original)
> +++ tomcat/tc6.0.x/trunk/STATUS.txt Tue Jun 28 00:08:55 2011
> @@ -169,7 +169,14 @@ PATCHES PROPOSED TO BACKPORT:
> http://svn.apache.org/viewvc?rev=1138950&view=rev
> http://svn.apache.org/viewvc?rev=1138953&view=rev
> +1: markt, schultz
> - -1:
> + -1: kkolinko: 1) It would be OK if it were mangling only reserved words
> + (where the tag wouldn't compile previously, so there were no regressions),
> + but JspUtils.makeJavaIdentifier() mangles '_' character, which will break
> + code that was previously working. E.g. <%=hello_world%>
> + 2) Maybe it would be better to prefix the names with [_]jsp_ and use some
> + numbered suffix, to never collide with names that people may use in their
> + code.
I don't see anything in the JSP specification that says tag file
attributes must be exposed as Java variables with the same name. Given
that a tag file attribute can have any name valid in XML, that
requirement would be impossible to meet since may of those names would
be invalid as Java identifiers.
There is a requirement to expose attributes (with the same name) as page
scoped variables. If I switch the test case to use ${hello_world} rather
than <%=hello_world%> it works. However, that causes it's own set of
problems when a Java keyword is used as an attribute name due to the
restrictions of the EL specification.
AFAICT, the only solution that is guaranteed (by the specification) to
work for any attribute name in a tag file is:
${pageScope['attributename']}
Unless I have missed something in the specification (always a
possibility) that anything else works is a fortunate side-effect of the
implementation.
On this basis, I think the new behaviour is compliant with the
specification.
That said, I take the point regarding regressions. I'm sure lots of
folks will have used <%=attributeName%> or ${attributeName} as a
shortcut although I don't currently see anything to support that usage
in the JSP specification.
I'll see what alternative solutions are available.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org