Implemention of variation trees#59
Conversation
ibbem
added
the
bm_thesis
|
Ok, we should merge the PR in Functjonal first. |
pmbittner
left a comment
pmbittner
left a comment
There was a problem hiding this comment.
The changes look awesome! Thank you. I still have to review VariationTree and VariationTreeNode which I will do asap.
| NodeType.values()[nodeTypeOrdinal], | ||
| from, | ||
| DiffLineNumber.InvalidLineNumber, | ||
| null, |
There was a problem hiding this comment.
Use true if we have an annotation node? Maybe its best to keep null here though as a "reminder" that this node is still incomplete.
There was a problem hiding this comment.
🤦 This is the exact same bug as I fixed in #50 (it caused the test failures). This needs to be fixed otherwise assertConsistency will detect that and we would have an unexpected inconsistent behavior between DiffTreen.assertConsistency and VariationNode.assertConsistency.
There was a problem hiding this comment.
Maybe we should document in the method's javadoc which information is lost and only recovered by heuristics (i.e., line numbers and formula of annotations)?
There was a problem hiding this comment.
I don't think it's a good idea to give definite answers to what exactly can be recovered from this ID because this would encourage using it for serialization. Misusing getID for serializing some fields of DiffNode was a mistake in my opinion. It's only real use case should be identifying a node uniquely in a tree.
It's probably a good idea to completely eliminate fromID in the future as it's not really stable across versions (I already changed the IDs multiple times) and implicitly looses information.
There was a problem hiding this comment.
I see your point. So the misuse would be any use of fromID but toID should be fine. I implemented these methods because of a use case were we had to export DiffTrees for another tool that reads linegraph files which in turn require natural number identifiers for nodes. I agree that upon serialisation we should either use a proper format that stores node data separately (e.g., in a json or in a text label in the linegraph file).
|
I am done with the first review now. |
ibbem
force-pushed
the
benjamin/variation-trees
branch
3 times, most recently
from
eb21341 to
df8c0a6
Compare
This also avoids duplicating `ID_OFFSET` in both `DiffNode` and `VariationTreeNode`.
It's last usage was removed in 4ac9cda.
This check will only be `true` if the root is not an `IF` node, but this is an invariant so it should never occur. It's better to get the resulting `NullPointerException` than a `null` return value.
See https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern for an explanation of the CRTP pattern.
ibbem
force-pushed
the
benjamin/variation-trees
branch
from
df8c0a6 to
a3a2a51
Compare
|
Thank you for all the fixes and adaptations. We can merge now. 🥳 |
2 participants
Implements variation trees and generalizes some algorithms by using projections of variation diffs.
There is some more refactoring potential here (for example
DiffTreeSourcecould use twoVariationTreeSources) and there are very few convenience methods inVariationTreein comparison toDiffTreebut this can be fixed later, when needed.Currently this adds
FilterMappedListto DiffDetective. If VariantSync/Functjonal#5 is merged before this one, I will instead update the Functjonal dependency in this pull request. Otherwise I will create a second one.