fix spurious circular references in routing policy called policy detection

[arifogel]

This change is 

[dhalperi]

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

a discussion (no related file):
Add a new unit test for the bug being fixed.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/CallExpr.java, line 43 at r1 (raw file):

    RoutingPolicy calledPolicy = routingPolicies.get(_calledPolicyName);
    if (calledPolicy == null) {
      return Collections.emptySet();

is this not a warning?

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/If.java, line 39 at r1 (raw file):

    ImmutableSet.Builder<String> childSources = ImmutableSet.builder();
    for (Statement statement : _falseStatements) {
      childSources.addAll(statement.collectSources(parentSources, routingPolicies, w).iterator());

why is .iterator() necessary? ImmutableSet.Builder has a #addAll(Iterable)

---

Comments from Reviewable

[arifogel]

Review status: 6 of 16 files reviewed at latest revision, 3 unresolved discussions.

---

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

Add a new unit test for the bug being fixed.

Done

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/CallExpr.java, line 43 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

is this not a warning?

Already handled by undefinedReferences question. I don't think we should warn every time.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/statement/If.java, line 39 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

why is .iterator() necessary? ImmutableSet.Builder has a #addAll(Iterable)

Hmm I might have confused with a different builder that only accepts iterator.
Fixed.

---

Comments from Reviewable

[dhalperi]

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/RoutingPolicyTests.java, line 8 at r2 (raw file):

import static org.junit.Assert.assertThat;

import jersey.repackaged.com.google.common.collect.ImmutableList;

import the Guava ImmutableList instead.

Does Eclipse not have a way to blacklist auto-imports?

---

Comments from Reviewable

[dhalperi]

Reviewed 10 of 10 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java, line 289 at r2 (raw file):

      }
    }
    // Compute sources for all other policies (previous ones are cached)

Proposing a small optional tweak might make it clearer.

Invert this: compute them all, then fill in where they're used.

---

Comments from Reviewable

[dhalperi]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[arifogel]

Review status: 14 of 16 files reviewed at latest revision, 2 unresolved discussions.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Configuration.java, line 289 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Proposing a small optional tweak might make it clearer.

Invert this: compute them all, then fill in where they're used.

done

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/routing_policy/RoutingPolicyTests.java, line 8 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

import the Guava ImmutableList instead.

Does Eclipse not have a way to blacklist auto-imports?

fixed for now, will look into eclipse way later

---

Comments from Reviewable

[dhalperi]

---

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable