You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Konstantin Kolinko <kn...@gmail.com> on 2014/01/21 05:14:40 UTC
Notes from review of BZ 56029 and ELParser patches
Hi!
Reviewing the patches for
https://issues.apache.org/bugzilla/show_bug.cgi?id=56029
(r1559555, r1559708, r1559820)
Important:
~~~~~~~~
1. The test "org.apache.jasper.compiler.TestELParser" tests success of
a roundtrip:
- parsing a String into EL expression
- recreating original String with ELParser$TextBuilder
The method ELParser$TextBuilder.visit(Function n) reconstructs function call as
prefix + ':' + name + '('
To reconstruct original string it has to know whitespaces of 4 tokens:
prefix, name and ':' and '('.
Starting with r1559708 both prefix and name are trimmed, and
character tokens do not know their whitespace either.
Thus it does not fly.
Test TestELParser.testTernary07() is a valid test, but it is a bit
misleading. It works because '$ {' does not start an EL expression.
If I remove the whitespace between '$ {' characters, its starts to fail.
The following 3 new tests fail:
doTestParser(" ${ do:it( a eq 1 ? true : false, y ) } ");
doTestParser(" ${ do:it ( a eq 1 ? true : false, y ) } ");
doTestParser(" ${!empty my:link(foo)} ");
(If reconstruction were not necessary then I think BZ 56029 could be
fixed by simply returning
' ' + prefix + ':' + name + '(' there. The leading whitespace can be
conditioned on whether
preceding character is a symbol.)
Performance notes:
~~~~~~~~~~~~~~~~~~~~
Performance of this code is not important, as it runs at JSP
compilation time only. I think compilation of the java code is the
main consumer of time here.
With such disclaimer, here are some notes.
2. The code that resets StringBuilder in methods parseEL() and
getAndResetWhiteSpace() of ELParser.
Currently it is done via "new StringBuilder()" call.
It can be done by calling buf.setLength(0). I see no need to create a
new buffer each time.
3. The input here is a String. When parsing a whitespace or a token
it would be better to call substring() method on the original String
rather than manipulating a StringBuilder.
A StringBuilder is needed only when some unescaping is performed and
thus substring() cannot be used.
4. Calling ELParser$Id.toString().trim() performs string
concatenation (whiteSpace+id) followed by trimming. The string
manipulations can be avoided by adding a new method that returns
trimmed text (the value of id).
The prefix + ':' + name + '(' concatenation in ELParser$TextBuilder
could be avoided if one had a substring() from the original input
String that contains all those spaces and tokens.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Notes from review of BZ 56029 and ELParser patches
Posted by Mark Thomas <ma...@apache.org>.
On 21/01/2014 04:14, Konstantin Kolinko wrote:
> Hi!
>
> Reviewing the patches for
> https://issues.apache.org/bugzilla/show_bug.cgi?id=56029
> (r1559555, r1559708, r1559820)
Thanks for the review. I'm really beginning to dislike this code ;)
I'll take a look at the individual issues asap.
As an aside, I'd love to let the spec compliant EL parser we already
have deal with this. Unfortunately, Jasper has to access it via the EL
API and that API does not provide enough access to the internals to
enable Jasper to do what it needs to do.
Mark
>
> Important:
> ~~~~~~~~
> 1. The test "org.apache.jasper.compiler.TestELParser" tests success of
> a roundtrip:
> - parsing a String into EL expression
> - recreating original String with ELParser$TextBuilder
>
> The method ELParser$TextBuilder.visit(Function n) reconstructs function call as
> prefix + ':' + name + '('
>
> To reconstruct original string it has to know whitespaces of 4 tokens:
> prefix, name and ':' and '('.
> Starting with r1559708 both prefix and name are trimmed, and
> character tokens do not know their whitespace either.
>
> Thus it does not fly.
>
> Test TestELParser.testTernary07() is a valid test, but it is a bit
> misleading. It works because '$ {' does not start an EL expression.
> If I remove the whitespace between '$ {' characters, its starts to fail.
> The following 3 new tests fail:
> doTestParser(" ${ do:it( a eq 1 ? true : false, y ) } ");
> doTestParser(" ${ do:it ( a eq 1 ? true : false, y ) } ");
> doTestParser(" ${!empty my:link(foo)} ");
>
> (If reconstruction were not necessary then I think BZ 56029 could be
> fixed by simply returning
> ' ' + prefix + ':' + name + '(' there. The leading whitespace can be
> conditioned on whether
> preceding character is a symbol.)
>
>
> Performance notes:
> ~~~~~~~~~~~~~~~~~~~~
> Performance of this code is not important, as it runs at JSP
> compilation time only. I think compilation of the java code is the
> main consumer of time here.
> With such disclaimer, here are some notes.
>
> 2. The code that resets StringBuilder in methods parseEL() and
> getAndResetWhiteSpace() of ELParser.
>
> Currently it is done via "new StringBuilder()" call.
>
> It can be done by calling buf.setLength(0). I see no need to create a
> new buffer each time.
>
> 3. The input here is a String. When parsing a whitespace or a token
> it would be better to call substring() method on the original String
> rather than manipulating a StringBuilder.
>
> A StringBuilder is needed only when some unescaping is performed and
> thus substring() cannot be used.
>
> 4. Calling ELParser$Id.toString().trim() performs string
> concatenation (whiteSpace+id) followed by trimming. The string
> manipulations can be avoided by adding a new method that returns
> trimmed text (the value of id).
>
> The prefix + ':' + name + '(' concatenation in ELParser$TextBuilder
> could be avoided if one had a substring() from the original input
> String that contains all those spaces and tokens.
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org