On 29/10/2011 01:55, kkoli...@apache.org 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: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org