Added .equals() and .hashCode() to all AbstractArray subclasses
#22
Conversation
…r, assert that it is true.
… has a cyclic graph in its objects which makes equallity testing especially difficult.
|
Thank you for the PR. I'll be travelling until next Thursday, but I'll try to take a look at it as soon as I can. |
…red in equality checks.
…ight not match the MatlabType of the semantic array.
| @@ -302,4 +303,18 @@ private static void checkMat5Identifier(String description) { | |||
|
|
|||
| private static final String MAT5_IDENTIFIER = "MATLAB 5.0 MAT-file"; | |||
|
|
|||
| @Override | |||
| protected int subHashCode() { | |||
| return Objects.hash(description, subsysOffset, byteOrder, version, reduced); | |||
There was a problem hiding this comment.
Project should be compatible with Java 6, but Objects was added in Java 7.
There was a problem hiding this comment.
Roger. Re: Java 6 compliance, is this a requirement for you, or is it a requirement to make it easier for others to adopt? I think Java 8 penetration is pretty good now (it was 75% in Feb 2018). Gradle 5 now requires Java 8.
Happy to stick to SE6 if you want :)
There was a problem hiding this comment.
We have an API that uses it and should be able to run in MATLAB 2012b (bundles a Java SE 6 runtime). Unless there is a significant upside to it, I'd prefer to not change the requirement to 2013b (bundles Java SE 7 runtime) quite yet.
| @@ -48,4 +52,73 @@ | |||
|
|
|||
| void writeMat5(Sink sink) throws IOException; | |||
|
|
|||
| static int hashCodeForType(@Nullable NumberStore store, boolean logical, MatlabType type) { | |||
There was a problem hiding this comment.
static interface methods not allowed pre-Java 8
| return hash; | ||
| } | ||
|
|
||
| static boolean equalForType(@Nullable NumberStore a, @Nullable NumberStore b, boolean logical, MatlabType type) { |
There was a problem hiding this comment.
static interface methods not allowed pre-Java 8
There was a problem hiding this comment.
Maybe this should just be an instance method in UniversalNumberStore::equals for now?
| } | ||
| } | ||
|
|
||
| protected abstract boolean subSubEquals(Object otherGuaranteedSameClass); |
There was a problem hiding this comment.
I can see a reason for the first sub- level, but nesting them seems a bit much. I'd prefer children to just override the subEqualsGuaranteedSameClass and call super if necessary.
If you feel strongly about it and really want to enforce explicit implementation, the AbstractStruct::subEqualsGuaranteedSameClass should be final.
There was a problem hiding this comment.
Roger. Subclassing and equality are tricky, so I like the final approach.
There was a problem hiding this comment.
Nvm, I think you are right that calling super. is better, I misunderstood the class hierarchy.
| // the object graph is not hierarchical or even a DAG. | ||
| // it is a cyclic graph which causes StackOverflow on equality check | ||
| // rare enough that it's not clear how much investigation is warranted... | ||
| boolean equalityCheck = false; |
There was a problem hiding this comment.
I'm ok punting on equality cycles.
|
|
||
| import java.io.Closeable; | ||
| import java.io.IOException; | ||
|
|
||
| import javax.annotation.Nullable; |
There was a problem hiding this comment.
Not available on maven build
…re for Java SE6 compat.
Also fixes a bug in MatObjectStruct's hashCode, because `subSubHashCode()` never actually got called.
|
Thanks for the feedback! I addressed all the points, ready for the next round :) |
|
Accepted. Thank you for the PR! The previous errors were related to |
|
I'm okay with Travis, but useless with maven ;-) Well done on MatFileRW, huge maintainability step up from MatFileRW. It looks ready for MatFileRW to point users to this. That okay with you? Happy to wait for 1.0 if you prefer :) |
Straightforward equality for all the
AbstractStructandAbstractMatFilesubclasses. The only wrinkle is that one of the MCOS examples actually creates a cyclic object graph, which causes the equality check to StackOverflow. A cyclic object graph inside a MAT-file seems like enough of a corner case that the feature is valuable on its own, not clear how much effort it's worth expending to dig into such a tiny case.