Mailing-List: contact dev-help@hc.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "HttpComponents Project" <dev@hc.apache.org>
Received-SPF: pass (athena.apache.org: domain of sebbaz@gmail.com designates
 74.125.82.51 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53E3C4F5.7020900@apache.org>
References: <20140806092940.690902389262@eris.apache.org>
	<CAOGo0Vb3nNursFDQcy050BJVz3fM_vsJAY0HakGJi4V_Tv7O9g@mail.gmail.com>
	<CAOGo0VY8QHSwXF5RvSC4gboGHX5WZ-6F0Do49Sj=HukLzneiQQ@mail.gmail.com>
	<53E3C4F5.7020900@apache.org>
Date: Thu, 7 Aug 2014 21:13:42 +0100
Message-ID: 
 <CAOGo0Va4ke1+uKvFvnY3MARS=6XAbfeibpaGHQRXjUJJd8NCUw@mail.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
From: sebb <sebbaz@gmail.com>
To: HttpComponents Project <dev@hc.apache.org>
Content-Type: text/plain; charset=UTF-8

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