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