From 8f995759e9c4e9254c555655ad696bf35698781a Mon Sep 17 00:00:00 2001 From: ashwinraghav Date: Fri, 5 Oct 2018 17:34:11 -0700 Subject: [PATCH 1/5] Enabling lint There are types of linting errors that seem worthy of being fixed. I have adopted some cookie cutter solution to fix two types of errors: 1) Remove usage of asserts that are unsupported in ART See: https://android.googlesource.com/platform/tools/base/+/master/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/AssertDetector.java 2) Replace HashMaps with SparseArrays Which seemed appropriate based on a rudimentary glance. See: https://stackoverflow.com/questions/25560629/sparsearray-vs-hashmap If you feel like these replacements are appropriate, I can replace them at a bunch of more places and enable linting --- firebase-firestore/firebase-firestore.gradle | 4 +--- .../core/number/IndexNumberEncoder.java | 11 +++++++--- .../core/number/NumberIndexEncoder.java | 10 ++++++--- .../firestore/remote/RemoteStore.java | 22 ++++++++++--------- 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/firebase-firestore/firebase-firestore.gradle b/firebase-firestore/firebase-firestore.gradle index 237cdee67d9..39a65b5ac7a 100644 --- a/firebase-firestore/firebase-firestore.gradle +++ b/firebase-firestore/firebase-firestore.gradle @@ -59,9 +59,7 @@ android { multiDexEnabled true testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" } - lintOptions { - abortOnError false - } + sourceSets { main { proto { diff --git a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java index 1f2cbb14d83..a0615fb7b83 100644 --- a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java +++ b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java @@ -14,6 +14,8 @@ package com.google.cloud.datastore.core.number; +import com.google.firebase.firestore.BuildConfig; + /** * Encodes numbers (longs and doubles) as bytes whose order matches the numeric order. * @@ -296,7 +298,8 @@ private static int encodeNumber( // Store exponents [4, 19] biased as [0, 15] exponent -= EXP1_END; - assert exponent <= 0xF; + if(BuildConfig.DEBUG && !(exponent <= 0xF)) + throw new RuntimeException(); // Store exponent as low order 4 bits, no continuation bit lastByte |= exponent; @@ -314,7 +317,8 @@ private static int encodeNumber( // Store exponents [20, 147] biased as [0, 127] exponent -= EXP2_END; - assert exponent <= 0x7F; + if(BuildConfig.DEBUG && !(exponent <= 0x7F)) + throw new RuntimeException(); // Pack the top 3 bits of the 7 bit exponent as the low order bits lastByte |= exponent >>> 4; @@ -342,7 +346,8 @@ private static int encodeNumber( // Store exponents [148, 1171] biased as [0, 1023] exponent -= EXP3_END; - assert exponent <= 0x3FF; + if(BuildConfig.DEBUG && !(exponent <= 0x3FF)) + throw new RuntimeException(); // Pack the top 2 bits of the exponent as the low order bits lastByte |= exponent >>> 8; diff --git a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java index 5df4dd001b1..072c0626719 100644 --- a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java +++ b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java @@ -16,6 +16,7 @@ import static com.google.cloud.datastore.core.number.NumberParts.SIGNIFICAND_BITS; +import com.google.firebase.firestore.BuildConfig; import java.util.Arrays; /** @@ -194,7 +195,8 @@ public static byte[] encode(NumberParts parts) { // Store exponents [4, 19] biased as [0, 15] exponent -= EXP1_END; - assert exponent <= 0xF; + if(BuildConfig.DEBUG && !(exponent <= 0xF)) + throw new RuntimeException(); // Store exponent as low order 4 bits, no continuation bit lastByte |= exponent; @@ -212,7 +214,8 @@ public static byte[] encode(NumberParts parts) { // Store exponents [20, 147] biased as [0, 127] exponent -= EXP2_END; - assert exponent <= 0x7F; + if(BuildConfig.DEBUG && !(exponent <= 0x7F)) + throw new RuntimeException(); // Pack the top 3 bits of the 7 bit exponent as the low order bits lastByte |= exponent >>> 4; @@ -240,7 +243,8 @@ public static byte[] encode(NumberParts parts) { // Store exponents [148, 1171] biased as [0, 1023] exponent -= EXP3_END; - assert exponent <= 0x3FF; + if(BuildConfig.DEBUG && !(exponent <= 0x3FF)) + throw new RuntimeException(); // Pack the top 2 bits of the exponent as the low order bits lastByte |= exponent >>> 8; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index 848280c9b47..5c2b4f709b4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import android.support.annotation.Nullable; +import android.util.SparseArray; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.OnlineState; import com.google.firebase.firestore.core.Transaction; @@ -118,7 +119,7 @@ public interface RemoteStoreCallback { * removed with unlistens are removed eagerly without waiting for confirmation from the listen * stream. */ - private final Map listenTargets; + private final SparseArray listenTargets; private final OnlineStateTracker onlineStateTracker; @@ -152,7 +153,7 @@ public RemoteStore( this.localStore = localStore; this.datastore = datastore; - listenTargets = new HashMap<>(); + listenTargets = new SparseArray(); writePipeline = new ArrayDeque<>(); onlineStateTracker = @@ -292,8 +293,7 @@ public void handleCredentialChange() { /** Listens to the target identified by the given QueryData. */ public void listen(QueryData queryData) { Integer targetId = queryData.getTargetId(); - hardAssert( - !listenTargets.containsKey(targetId), + hardAssert(listenTargets.get(targetId) == null, "listen called with duplicate target ID: %d", targetId); @@ -318,7 +318,8 @@ private void sendWatchRequest(QueryData queryData) { * be torn down after one minute of inactivity. */ public void stopListening(int targetId) { - QueryData queryData = listenTargets.remove(targetId); + QueryData queryData = listenTargets.get(targetId); + listenTargets.remove(targetId); hardAssert( queryData != null, "stopListening called on target no currently watched: %d", targetId); @@ -327,7 +328,7 @@ public void stopListening(int targetId) { sendUnwatchRequest(targetId); } - if (listenTargets.isEmpty()) { + if (listenTargets.size()==0) { if (watchStream.isOpen()) { watchStream.markIdle(); } else if (this.canUseNetwork()) { @@ -357,7 +358,7 @@ private boolean shouldStartWriteStream() { * active watch targets. */ private boolean shouldStartWatchStream() { - return canUseNetwork() && !watchStream.isStarted() && !listenTargets.isEmpty(); + return canUseNetwork() && !watchStream.isStarted() && listenTargets.size() != 0; } private void cleanUpWatchStreamState() { @@ -378,9 +379,10 @@ private void startWatchStream() { } private void handleWatchStreamOpen() { + final int nsize = listenTargets.size(); // Restore any existing watches. - for (QueryData queryData : listenTargets.values()) { - sendWatchRequest(queryData); + for(int i = 0; i < nsize; i++) { + sendWatchRequest(listenTargets.get(i)); } } @@ -515,7 +517,7 @@ private void processTargetError(WatchTargetChange targetChange) { hardAssert(targetChange.getCause() != null, "Processing target error without a cause"); for (Integer targetId : targetChange.getTargetIds()) { // Ignore targets that have been removed already. - if (listenTargets.containsKey(targetId)) { + if (listenTargets.get(targetId) != null) { listenTargets.remove(targetId); watchChangeAggregator.removeTarget(targetId); remoteStoreCallback.handleRejectedListen(targetId, targetChange.getCause()); From 0dfd8684fab6c60eea1cf386f45b4ea34d39374c Mon Sep 17 00:00:00 2001 From: ashwinraghav Date: Mon, 8 Oct 2018 14:24:55 -0700 Subject: [PATCH 2/5] Revert "Enabling lint" This reverts commit 8f995759e9c4e9254c555655ad696bf35698781a. --- firebase-firestore/firebase-firestore.gradle | 4 +++- .../core/number/IndexNumberEncoder.java | 11 +++------- .../core/number/NumberIndexEncoder.java | 10 +++------ .../firestore/remote/RemoteStore.java | 22 +++++++++---------- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/firebase-firestore/firebase-firestore.gradle b/firebase-firestore/firebase-firestore.gradle index 39a65b5ac7a..237cdee67d9 100644 --- a/firebase-firestore/firebase-firestore.gradle +++ b/firebase-firestore/firebase-firestore.gradle @@ -59,7 +59,9 @@ android { multiDexEnabled true testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" } - + lintOptions { + abortOnError false + } sourceSets { main { proto { diff --git a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java index a0615fb7b83..1f2cbb14d83 100644 --- a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java +++ b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/IndexNumberEncoder.java @@ -14,8 +14,6 @@ package com.google.cloud.datastore.core.number; -import com.google.firebase.firestore.BuildConfig; - /** * Encodes numbers (longs and doubles) as bytes whose order matches the numeric order. * @@ -298,8 +296,7 @@ private static int encodeNumber( // Store exponents [4, 19] biased as [0, 15] exponent -= EXP1_END; - if(BuildConfig.DEBUG && !(exponent <= 0xF)) - throw new RuntimeException(); + assert exponent <= 0xF; // Store exponent as low order 4 bits, no continuation bit lastByte |= exponent; @@ -317,8 +314,7 @@ private static int encodeNumber( // Store exponents [20, 147] biased as [0, 127] exponent -= EXP2_END; - if(BuildConfig.DEBUG && !(exponent <= 0x7F)) - throw new RuntimeException(); + assert exponent <= 0x7F; // Pack the top 3 bits of the 7 bit exponent as the low order bits lastByte |= exponent >>> 4; @@ -346,8 +342,7 @@ private static int encodeNumber( // Store exponents [148, 1171] biased as [0, 1023] exponent -= EXP3_END; - if(BuildConfig.DEBUG && !(exponent <= 0x3FF)) - throw new RuntimeException(); + assert exponent <= 0x3FF; // Pack the top 2 bits of the exponent as the low order bits lastByte |= exponent >>> 8; diff --git a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java index 072c0626719..5df4dd001b1 100644 --- a/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java +++ b/firebase-firestore/src/main/java/com/google/cloud/datastore/core/number/NumberIndexEncoder.java @@ -16,7 +16,6 @@ import static com.google.cloud.datastore.core.number.NumberParts.SIGNIFICAND_BITS; -import com.google.firebase.firestore.BuildConfig; import java.util.Arrays; /** @@ -195,8 +194,7 @@ public static byte[] encode(NumberParts parts) { // Store exponents [4, 19] biased as [0, 15] exponent -= EXP1_END; - if(BuildConfig.DEBUG && !(exponent <= 0xF)) - throw new RuntimeException(); + assert exponent <= 0xF; // Store exponent as low order 4 bits, no continuation bit lastByte |= exponent; @@ -214,8 +212,7 @@ public static byte[] encode(NumberParts parts) { // Store exponents [20, 147] biased as [0, 127] exponent -= EXP2_END; - if(BuildConfig.DEBUG && !(exponent <= 0x7F)) - throw new RuntimeException(); + assert exponent <= 0x7F; // Pack the top 3 bits of the 7 bit exponent as the low order bits lastByte |= exponent >>> 4; @@ -243,8 +240,7 @@ public static byte[] encode(NumberParts parts) { // Store exponents [148, 1171] biased as [0, 1023] exponent -= EXP3_END; - if(BuildConfig.DEBUG && !(exponent <= 0x3FF)) - throw new RuntimeException(); + assert exponent <= 0x3FF; // Pack the top 2 bits of the exponent as the low order bits lastByte |= exponent >>> 8; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index 5c2b4f709b4..848280c9b47 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -17,7 +17,6 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import android.support.annotation.Nullable; -import android.util.SparseArray; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.OnlineState; import com.google.firebase.firestore.core.Transaction; @@ -119,7 +118,7 @@ public interface RemoteStoreCallback { * removed with unlistens are removed eagerly without waiting for confirmation from the listen * stream. */ - private final SparseArray listenTargets; + private final Map listenTargets; private final OnlineStateTracker onlineStateTracker; @@ -153,7 +152,7 @@ public RemoteStore( this.localStore = localStore; this.datastore = datastore; - listenTargets = new SparseArray(); + listenTargets = new HashMap<>(); writePipeline = new ArrayDeque<>(); onlineStateTracker = @@ -293,7 +292,8 @@ public void handleCredentialChange() { /** Listens to the target identified by the given QueryData. */ public void listen(QueryData queryData) { Integer targetId = queryData.getTargetId(); - hardAssert(listenTargets.get(targetId) == null, + hardAssert( + !listenTargets.containsKey(targetId), "listen called with duplicate target ID: %d", targetId); @@ -318,8 +318,7 @@ private void sendWatchRequest(QueryData queryData) { * be torn down after one minute of inactivity. */ public void stopListening(int targetId) { - QueryData queryData = listenTargets.get(targetId); - listenTargets.remove(targetId); + QueryData queryData = listenTargets.remove(targetId); hardAssert( queryData != null, "stopListening called on target no currently watched: %d", targetId); @@ -328,7 +327,7 @@ public void stopListening(int targetId) { sendUnwatchRequest(targetId); } - if (listenTargets.size()==0) { + if (listenTargets.isEmpty()) { if (watchStream.isOpen()) { watchStream.markIdle(); } else if (this.canUseNetwork()) { @@ -358,7 +357,7 @@ private boolean shouldStartWriteStream() { * active watch targets. */ private boolean shouldStartWatchStream() { - return canUseNetwork() && !watchStream.isStarted() && listenTargets.size() != 0; + return canUseNetwork() && !watchStream.isStarted() && !listenTargets.isEmpty(); } private void cleanUpWatchStreamState() { @@ -379,10 +378,9 @@ private void startWatchStream() { } private void handleWatchStreamOpen() { - final int nsize = listenTargets.size(); // Restore any existing watches. - for(int i = 0; i < nsize; i++) { - sendWatchRequest(listenTargets.get(i)); + for (QueryData queryData : listenTargets.values()) { + sendWatchRequest(queryData); } } @@ -517,7 +515,7 @@ private void processTargetError(WatchTargetChange targetChange) { hardAssert(targetChange.getCause() != null, "Processing target error without a cause"); for (Integer targetId : targetChange.getTargetIds()) { // Ignore targets that have been removed already. - if (listenTargets.get(targetId) != null) { + if (listenTargets.containsKey(targetId)) { listenTargets.remove(targetId); watchChangeAggregator.removeTarget(targetId); remoteStoreCallback.handleRejectedListen(targetId, targetChange.getCause()); From 300b84e2e8c1c7a5de264f54df4d2b5e447ce2c6 Mon Sep 17 00:00:00 2001 From: ashwinraghav Date: Mon, 8 Oct 2018 15:09:15 -0700 Subject: [PATCH 3/5] Specific whitelisting of lint whitelists --- firebase-firestore/firebase-firestore.gradle | 4 +--- firebase-firestore/lint.xml | 21 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 firebase-firestore/lint.xml diff --git a/firebase-firestore/firebase-firestore.gradle b/firebase-firestore/firebase-firestore.gradle index 237cdee67d9..39a65b5ac7a 100644 --- a/firebase-firestore/firebase-firestore.gradle +++ b/firebase-firestore/firebase-firestore.gradle @@ -59,9 +59,7 @@ android { multiDexEnabled true testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" } - lintOptions { - abortOnError false - } + sourceSets { main { proto { diff --git a/firebase-firestore/lint.xml b/firebase-firestore/lint.xml new file mode 100644 index 00000000000..6893ad35504 --- /dev/null +++ b/firebase-firestore/lint.xml @@ -0,0 +1,21 @@ + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file From a798f83643a1d413da6f21cf3693e2fece4df701 Mon Sep 17 00:00:00 2001 From: ashwinraghav Date: Mon, 8 Oct 2018 15:32:55 -0700 Subject: [PATCH 4/5] Whitelisting LruGarbageCollector --- firebase-firestore/lint.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/firebase-firestore/lint.xml b/firebase-firestore/lint.xml index 6893ad35504..e063062af2f 100644 --- a/firebase-firestore/lint.xml +++ b/firebase-firestore/lint.xml @@ -1,5 +1,11 @@ + + + + + + From f3d81a97063fd5796802700671364961ec37428b Mon Sep 17 00:00:00 2001 From: ashwinraghav Date: Mon, 8 Oct 2018 15:43:19 -0700 Subject: [PATCH 5/5] Whitelisting src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java --- firebase-firestore/lint.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/lint.xml b/firebase-firestore/lint.xml index e063062af2f..6fb92c28721 100644 --- a/firebase-firestore/lint.xml +++ b/firebase-firestore/lint.xml @@ -3,7 +3,7 @@ - +