hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: Fwd: svn commit: r1616136 - in /httpcomponents/httpclient/trunk/httpclient/src: main/java/org/apache/http/conn/ssl/DistinguishedNameParser.java test/java/org/apache/http/conn/ssl/TestDistinguishedNameParser.java
Date Thu, 07 Aug 2014 20:13:42 GMT
On 7 August 2014 19:27, Oleg Kalnichevski <olegk@apache.org> wrote:
> On 07/08/14 17:26 , sebb wrote:
>>
>> Again, looks generally OK.
>>
>> Some comments below.
>>
>> On 6 August 2014 10:29,  <olegk@apache.org> wrote:
>>>
>>> +@Immutable
>>
>>
>> Are we sure it's immutable?
>>
>>> +final class DistinguishedNameParser {
>>> +
>>> +    public final static DistinguishedNameParser INSTANCE = new
>>> DistinguishedNameParser();
>>> +
>>> +    private static final BitSet EQUAL_OR_COMMA_OR_PLUS      =
>>> TokenParser.INIT_BITSET('=', ',', '+');
>>> +    private static final BitSet COMMA_OR_PLUS               =
>>> TokenParser.INIT_BITSET(',', '+');
>>
>>
>> BitSet is not immutable, so using a shared static field relies on the
>> code never calling any of the mutation methods after creation.
>>
>
> True, but these static variables are private and their state does not mutate
> post construction.

At present.

>
>> Perhaps consider using a simple delegation wrapper to provide only read
>> access?
>> Otherwise we need to document the non-mutability assumption.
>>
>
> We could but given those static variables are private I see no real need to
> do so.

It still needs to be documented for future maintainers.

>
>>> +
>>> +    static class InternalTokenParser extends TokenParser {
>>> +
>>
>>
>> Do we really need this?
>> Perhaps escape processing should be merged into the super-class.
>>
>
> Yes, we do. HTTP spec states that escaped chars are valid only when enclosed
> in double quotes. It is quite different from RFC 4514 and earlier RFCs
> superseded by it.

>
>
>> But if it is kept, it needs to have Javadoc.
>>
>
> It is a package private class. Does this make any sense?

I know it is a local class, but that does not mean it should not be documented.

You have explained (above) why the super-class does not allow escapes
in unquoted text, but we need to document why the InternalTokenParser
needs to override that behaviour.

> Oleg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message