From af8bc1d9cd308e3d4f52330f740725c5c7847f1d Mon Sep 17 00:00:00 2001
From: Assaf <asafyi@gmail.com>
Date: Sun, 8 Jul 2018 16:01:34 +0300
Subject: [PATCH] Refactor LocatedItems::mergeItemsIntoNext Add test for
 mergeInto to make sure the order is kept.

---
 .../jf/dexlib2/builder/LocatedDebugItems.java |  2 +-
 .../org/jf/dexlib2/builder/LocatedItems.java  | 51 +++++++++++--------
 .../org/jf/dexlib2/builder/LocatedLabels.java |  2 +-
 .../jf/dexlib2/builder/MethodLocation.java    |  7 ++-
 .../jf/dexlib2/builder/LocatedItemsTest.java  | 46 +++++++++++++++++
 5 files changed, 81 insertions(+), 27 deletions(-)
 create mode 100644 dexlib2/src/test/java/org/jf/dexlib2/builder/LocatedItemsTest.java

diff --git a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedDebugItems.java b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedDebugItems.java
index 61403565..77291ac8 100644
--- a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedDebugItems.java
+++ b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedDebugItems.java
@@ -3,7 +3,7 @@ package org.jf.dexlib2.builder;
 public class LocatedDebugItems extends LocatedItems<BuilderDebugItem> {
 
     @Override
-    protected String addLocatedItemError() {
+    protected String getAddLocatedItemError() {
         return "Cannot add a debug item that has already been added to a method." +
                 "You must remove it from its current location first.";
     }
diff --git a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedItems.java b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedItems.java
index c4abf9e3..261742f9 100644
--- a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedItems.java
+++ b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedItems.java
@@ -13,16 +13,14 @@ public abstract class LocatedItems<T extends ItemWithLocation> {
     @Nullable
     private List<T> items = null;
 
-    @Nonnull
-    private List<T> getMutableItems() {
+    private void initItemsIfNull() {
         if (items == null) {
             items = new ArrayList<>(1);
         }
-        return items;
     }
 
     @Nonnull
-    private List<T> getImmutableItems() {
+    private List<T> getItems() {
         if (items == null) {
             return ImmutableList.of();
         }
@@ -32,23 +30,27 @@ public abstract class LocatedItems<T extends ItemWithLocation> {
     public Set<T> getModifiableItems(MethodLocation newItemsLocation) {
         return new AbstractSet<T>() {
             @Nonnull
-            @Override public Iterator<T> iterator() {
-                final Iterator<T> it = getImmutableItems().iterator();
+            @Override
+            public Iterator<T> iterator() {
+                final Iterator<T> it = getItems().iterator();
 
                 return new Iterator<T>() {
                     private @Nullable
                     T currentItem = null;
 
-                    @Override public boolean hasNext() {
+                    @Override
+                    public boolean hasNext() {
                         return it.hasNext();
                     }
 
-                    @Override public T next() {
+                    @Override
+                    public T next() {
                         currentItem = it.next();
                         return currentItem;
                     }
 
-                    @Override public void remove() {
+                    @Override
+                    public void remove() {
                         if (currentItem != null) {
                             currentItem.setLocation(null);
                         }
@@ -57,30 +59,37 @@ public abstract class LocatedItems<T extends ItemWithLocation> {
                 };
             }
 
-            @Override public int size() {
-                return getImmutableItems().size();
+            @Override
+            public int size() {
+                return getItems().size();
             }
 
-            @Override public boolean add(@Nonnull T item) {
+            @Override
+            public boolean add(@Nonnull T item) {
                 if (item.isPlaced()) {
-                    throw new IllegalArgumentException(addLocatedItemError());
+                    throw new IllegalArgumentException(getAddLocatedItemError());
                 }
                 item.setLocation(newItemsLocation);
-                getMutableItems().add(item);
+                initItemsIfNull();
+                items.add(item);
                 return true;
             }
         };
     }
 
-    protected abstract String addLocatedItemError();
+    protected abstract String getAddLocatedItemError();
 
-    public void mergeItemsInto(@Nonnull MethodLocation newLocation, LocatedItems<T> otherLocatedItems) {
-        if (items != null || otherLocatedItems.items != null) {
-            List<T> otherItems = otherLocatedItems.getMutableItems();
-            for (T item: getImmutableItems()) {
-                item.setLocation(newLocation);
-                otherItems.add(item);
+    public void mergeItemsIntoNext(@Nonnull MethodLocation nextLocation, LocatedItems<T> otherLocatedItems) {
+        if (otherLocatedItems == this) {
+            return;
+        }
+        if (items != null) {
+            for (T item : items) {
+                item.setLocation(nextLocation);
             }
+            List<T> mergedItems = items;
+            mergedItems.addAll(otherLocatedItems.getItems());
+            otherLocatedItems.items = mergedItems;
             items = null;
         }
     }
diff --git a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedLabels.java b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedLabels.java
index 2b7823f3..d14ce7bc 100644
--- a/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedLabels.java
+++ b/dexlib2/src/main/java/org/jf/dexlib2/builder/LocatedLabels.java
@@ -2,7 +2,7 @@ package org.jf.dexlib2.builder;
 
 public class LocatedLabels extends LocatedItems<Label> {
     @Override
-    protected String addLocatedItemError() {
+    protected String getAddLocatedItemError() {
         return "Cannot add a label that is already placed." +
                 "You must remove it from its current location first.";
     }
diff --git a/dexlib2/src/main/java/org/jf/dexlib2/builder/MethodLocation.java b/dexlib2/src/main/java/org/jf/dexlib2/builder/MethodLocation.java
index ea1fb7ee..da746e13 100644
--- a/dexlib2/src/main/java/org/jf/dexlib2/builder/MethodLocation.java
+++ b/dexlib2/src/main/java/org/jf/dexlib2/builder/MethodLocation.java
@@ -46,7 +46,6 @@ public class MethodLocation {
     int index;
 
     private final LocatedItems<Label> labels;
-    @Nullable
     private final LocatedItems<BuilderDebugItem> debugItems;
 
     MethodLocation(@Nullable BuilderInstruction instruction, int codeAddress, int index) {
@@ -70,9 +69,9 @@ public class MethodLocation {
         return index;
     }
 
-    void mergeInto(@Nonnull MethodLocation other) {
-        labels.mergeItemsInto(other, other.labels);
-        debugItems.mergeItemsInto(other, other.debugItems);
+    void mergeInto(@Nonnull MethodLocation nextLocation) {
+        labels.mergeItemsIntoNext(nextLocation, nextLocation.labels);
+        debugItems.mergeItemsIntoNext(nextLocation, nextLocation.debugItems);
     }
 
     @Nonnull
diff --git a/dexlib2/src/test/java/org/jf/dexlib2/builder/LocatedItemsTest.java b/dexlib2/src/test/java/org/jf/dexlib2/builder/LocatedItemsTest.java
new file mode 100644
index 00000000..a1947997
--- /dev/null
+++ b/dexlib2/src/test/java/org/jf/dexlib2/builder/LocatedItemsTest.java
@@ -0,0 +1,46 @@
+package org.jf.dexlib2.builder;
+
+import com.google.common.collect.Sets;
+import org.jf.dexlib2.builder.debug.BuilderLineNumber;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class LocatedItemsTest {
+
+    private List<BuilderDebugItem> createItems(int count) {
+        List<BuilderDebugItem> items = new ArrayList<>();
+        for(int i = 0; i < count; ++i) {
+            items.add(new BuilderLineNumber(i));
+        }
+        return items;
+    }
+
+    private void doTestMergeIntoKeepsOrderOfDebugItems(int countLocation1, int countLocation2) {
+        MethodLocation location1 = new MethodLocation(null, 123, 1);
+        MethodLocation location2 = new MethodLocation(null, 456, 2);
+
+        List<BuilderDebugItem> items1 = createItems(countLocation1);
+        List<BuilderDebugItem> items2 = createItems(countLocation2);
+        location1.getDebugItems().addAll(items1);
+        location2.getDebugItems().addAll(items2);
+
+        location1.mergeInto(location2);
+
+        Assert.assertEquals(Sets.newHashSet(), location1.getDebugItems());
+        // items1 appear BEFORE items2
+        List<BuilderDebugItem> expectedItems = new ArrayList<>(items1);
+        expectedItems.addAll(items2);
+        Assert.assertEquals(expectedItems, new ArrayList<>(location2.getDebugItems()));
+    }
+
+    @Test
+    public void testMergeIntoKeepsOrderOfDebugItems() {
+        doTestMergeIntoKeepsOrderOfDebugItems(2, 2);
+        doTestMergeIntoKeepsOrderOfDebugItems(0, 0);
+        doTestMergeIntoKeepsOrderOfDebugItems(0, 2);
+        doTestMergeIntoKeepsOrderOfDebugItems(2, 0);
+    }
+}