commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [codec] Generics added in a SVN branch
Date Mon, 15 Aug 2011 14:12:17 GMT
On 14 August 2011 22:38, Gary Gregory <garydgregory@gmail.com> wrote:
> Hi,
>
> On Aug 14, 2011, at 10:19, sebb <sebbaz@gmail.com> wrote:
>
>> On 14 August 2011 03:02, sebb <sebbaz@gmail.com> wrote:
>>> On 12 August 2011 20:56, Gary Gregory <garydgregory@gmail.com> wrote:
>>>> Hello All:
>>>>
>>>> For a first cut at generics support in Codec, please checkout the
>>>> branch https://svn.apache.org/repos/asf/commons/proper/codec/branches/generics
>>>>
>>>> I wrote a migration guide in the root of the project called
>>>> Codec2-Migration.htm.
>>>>
>>>> Let's discuss.
>>>
>>> The original code used Encoder and Decoder interfaces which operated
>>> on Object, and extended these with
>>> BinaryEncoder (byte[])
>>> StringEncoder(String)
>>>
>>> So for example StringEncoder classes need to implement
>>> encode(Object) : Object
>>> encode(String) : String
>>>
>>> As far as I can tell, those interfaces cannot be directly generified,
>>> because type erasure causes a method signature clash.
>>> So adding generics here means breaking binary compatibilty.
>>>
>>> Question is, what does adding generics to these interfaces actually provide?
>>> Would it not be just as effective to deprecate Decoder and Encoder,
>>> and code can then use either BinaryEncoder or StringEncoder (etc)?
>>>
>>> At the moment test code is of the form:
>>>
>>> Encoder enc = new Base64();
>>> ...
>>> byte [] ba = (byte[]) enc.encode(binary)); // unchecked cast
>>>
>>> However this could be written:
>>>
>>> BinaryEncoder enc = new Base64();
>>> ...
>>> byte [] ba = enc.encode(binary)); // no cast needed
>>>
>>
>> Note that the Encoder/Decoder interfaces do not _require_ generification.
>>
>
> Well sure. Strictly speaking that's true but I still like the branch
> code better.
>
>> The only non-generified part of trunk is StringEncoderComparator.
>>
>
> I do not quite look at it that way, since I created the branch ;) if
> you just want to fix warnings then yes that class needs patching. But
> my claim in the branch is that the component is better and type-safe
> with generics.
>
>> This appears from the name as if it compares Strings, but in fact it
>> compares Objects in the current code.
>> Making it implement Comparator<Object> fixes the warning without
>> compromising binary (or source?) compatibility.
>>
>> The StringEncoders generate an EncoderException if the parameter is
>> not a String.
>> This does mean that comparison with a non-String will return 0 (equal)
>> which is rather strange, but that is the way that the code current
>> works.
>
> That sure sounds like a bug.

Well, the method .StringEncoderComparator.compare(Object, Object) Javadoc says:

@return the Comparable.compareTo() return code or 0 if an encoding
error was caught.

Does seem like a design bug, as one cannot distinguish failure from equality.

>>
>> So what I suggest is - let's release Codec as a binary compatible
>> implementation, at least for now.
>
> ATM there incompat changes due to deprecations and other bits.

Yes, but those could be reverted if necessary.

>>
>> If we do decide to break binary compatibility in a later release, then
>> I think the Encoder/Decoder hierarchy needs a bit more work.
>> For example, several of the new xxxCodec classes only implement
>> Decoder, not Encoder; this is because of the need to avoid erasure
>> clashes.
>
> Another set of refinements the ;)

I don't think the redesign goes quite far enough to fix the interface
inheritance problem.

What I'm suggesting is to take smaller steps to achieve the goal.

Release a binary-compatible version now - which has lots of other
useful fixes - and release a completely new version later.
If problems are found with the new stuff, any API changes can be
rolled into the next major version.

>>
>> I just wonder if trying to use generics here is not causing more
>> problems than it solves?
>> It's not as if there are lots of different types that are used here -
>> only String, byte[] and char[].
>> Perhaps we could just add CharEncoder/CharDecoder interfaces instead?
>
> Please see my proposal for the typing of the new BM encoder which I
> want to type as Set<String> encode(String) in a previous email.
>
> Thank you,
> Gary
>
>>
>>>> I plan on not touching trunk until the generics code is flushed out
>>>> and brought back to trunk.
>>>>
>>>> Thank you,
>>>> Gary
>>>>
>>>> --
>>>> http://garygregory.wordpress.com/
>>>> http://garygregory.com/
>>>> http://people.apache.org/~ggregory/
>>>> http://twitter.com/GaryGregory
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

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


Mime
View raw message