On 29/10/2011 01:55, [email protected] wrote:
> Author: kkolinko
> Date: Sat Oct 29 00:55:28 2011
> New Revision: 1190720
>
> URL: http://svn.apache.org/viewvc?rev=1190720&view=rev
> Log:
> Add one more patch to Mark's proposal.
> Veto and add review comments.
Thanks for the comments. Fixes applied to trunk and 7.0.x and updated
proposals provided for 6.0.x and 5.5.x. More detail in-line.
> + kkolinko: In B2CConverter.java
> + lines 117-118: restore Javadoc comment to method void convert( ByteChunk
> bb, CharChunk cb, int limit)
Improved comment added to all versions
> + line 125 - remove "// conv.ready() ) {" comment. It wasn't in 5.5. Do
> not see why to add it.
Removed from all versions
> + In ByteChunk.java:
> + in toStringInternal():
> + "cb.array()" returns "character array that backs this CharBuffer".
> + I think it can contain extra characters beyond end of decoded string.
> + Thus "new String(cb.array());" is wrong.
> + Single-character charsets are simple, because the count of characters
> + is known from the count of bytes. Needs more testing with UTF-8.
> + - thus -1.
I have added the start and end to the new String() call.
I have added some UTF-8 parameters and values to the tests.
> + In Parameters.java:
> + "private static org.apache.commons.logging.Log log"
> + - make "static final"
Done.
> + "StringManager.getManager("org.apache.tomcat.util.http");"
> + - the LocalStrings.properties file from r1189882 is not included in
> this patch.
> + (2011-10-27-param-perf-tc6-v1.patch does not have it as well)
Done.
> + "private final Hashtable paramHashValues"
> + - Maybe a HashMap can be used instead. I do not expect much
> improvements
> + from that though.
Hashtable makes implementing getParameterNames() a lot simpler.
> + in #addParameterValues(..)
> + - Replace "values = new ArrayList(1);"
> + with "values = new ArrayList(newValues.length);"
Done along with a related tweak.
> + - Maybe "if (paramHashValues.containsKey(key))" can be replaced
> + with testing whether result of (paramHashValues.containsKey(key)) is
> null.
Hashtable does not permit null keys or values.
> + - Maybe add tests for this method. In trunk it is called by
> Request.parseParts(), though see below maybe that can be changed.
Done
> + in #getParameterValues(String name)
> + - Apply NPE fix from http://svn.apache.org/viewvc?rev=1190481&view=rev
Done
> + - Maybe add test case in TestParameters.java for calling
> getParameterValues() for non-existing parameter.
Done.
> + in #addParam(String, String)
> + - remove efficiency comment above the method?
Done.
> + - In trunk: maybe make this method public and call it instead of
> #addParameterValues(..)
> + in Request.parseParts().
I'll do that separately for trunk.
> + - Maybe "if (paramHashValues.containsKey(key))" can be replaced
> + with testing whether result of (paramHashValues.containsKey(key)) is
> null.
See previous comment re Hashtable.
> + "public static final String DEFAULT_ENCODING"
> + "public static final Charset DEFAULT_CHARSET"
> + - New fields. They can be made private.
Done.
> + in #processParameters(..., Charset)
> + - It is a new method. Maybe make it private.
Done.
> + in #urlDecode(...)
> + - Maybe call "bc.setCharset(charset);" before calling
> "urlDec.convert(bc);"
No need. convert is %nn->byte conversion
> + in #paramsAsString()
> + - Move sb.append("\n"); outside of loop that iterates on values. Those
> are separated by ','.
> + In the old code the "\n" is used to separate different parameter
> names.
Done.
> + - Replace double quotes with single quotes where it is a single
> character ('=', ',', '\n')
Done.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
