jasonjack2015 / protobuf

Automatically exported from code.google.com/p/protobuf
Other
0 stars 0 forks source link

Remove clone() from all interfaces, it is wrong; make them extend Cloneable, instead. #458

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Here is the example:

public class C1<T extends C1.I1 & C1.I2> {
  public interface I1 {}

  public interface I2 {
    I2 clone();
  }
}

The code above will produce a compilation error regarding clone() hiding.
However, the code below is okay (and also is in line with Java cloning 
semantics):

public class C1<T extends C1.I1 & C1.I2> {
  public interface I1 {}

  public interface I2 extends Cloneable {}
}

The only remedy to this compiler error is to make a combined interface and use 
it instead of "&" in generics:

public class C1<T extends C1.I3> {
  public interface I1 {}

  public interface I2 {
    I2 clone();
  }

  public interface I3 extends I1, I2 {}
}

In other words, the inclusion of the clone() method in an interface, prevents 
combining this interface with any other (and with any classes) using the "&" 
generics operator as shown above. The Message.Builder and the likes are just a 
concrete example of this design anti-pattern. 
  And the right remedy is to remove the offending clone() method from all interfaces and make them extend the Cloneable interface. In addition, it will require casting after use of clone methods, or classes can freely override it, but NOT interfaces!  

What version of the product are you using? On what operating system?
I observe this on Java 1.5 - 1.7. on Windows (should be the same on any OS).

I understand the impact, but the present and future pain will only increase 
with time.

Sincerely,
David

Original issue reported on code.google.com by dav.co...@gmail.com on 23 Jan 2013 at 11:26

GoogleCodeExporter commented 9 years ago
Actually, the problem is in Java itself. The Object#clone() method declared as 
protected, and this causes "&" operator to fail. The easiest (but not the most 
elegant solution) is to have some T cloneMe() method in the interfaces. If 
Cloneable is also desirable, it's fine. The implementing class then supposed to 
implement both methods, clone() and cloneMe(). Also, in this case clone() 
method might be declared protected to avoid the confusing of having both public 
methods doing essentially the same. 

Original comment by dav.co...@gmail.com on 9 Feb 2013 at 6:39

GoogleCodeExporter commented 9 years ago
The MessageLite.Builder already implements Cloneable, so only the removal of 
clone() method from all Builder interfaces is required.

Original comment by dav.co...@gmail.com on 13 Feb 2013 at 3:10

GoogleCodeExporter commented 9 years ago
Also, this AbstractMessage.Builder snippet:

// The compiler produces an error if this is not declared explicitly.
@Override
public abstract BuilderType clone();

by removing the clone() method from the Builder interfaces, the code above 
should be sufficient and without any compilation errors. Thus, all 
AbstractMessage.Builder subclasses, including DynamicMessage.Builder and 
GeneratedMessage.Builder ones, will work with clone() as-is; problem solved! 

Original comment by dav.co...@gmail.com on 13 Feb 2013 at 3:55