On 7 August 2014 19:27, Oleg Kalnichevski <[email protected]> wrote:
> On 07/08/14 17:26 , sebb wrote:
>>
>> Again, looks generally OK.
>>
>> Some comments below.
>>
>> On 6 August 2014 10:29, <[email protected]> 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: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
