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