SONARJS-627 Fix quality flaws and add comments

pynicolas · vilchik-elena · commit 0bdd5e389392 · 2016-02-23T12:18:13.000+01:00
diff --git a/javascript-checks/src/main/java/org/sonar/javascript/checks/UnreachableCodeCheck.java b/javascript-checks/src/main/java/org/sonar/javascript/checks/UnreachableCodeCheck.java
@@ -95,7 +95,7 @@ private void check(ControlFlowGraph cfg) {
     }
   }
 
-  private Tree unreachableTree(List<Tree> elements) {
+  private static Tree unreachableTree(List<Tree> elements) {
     for (Tree element : elements) {
       if (!element.is(Kind.FUNCTION_DECLARATION, Kind.GENERATOR_DECLARATION, Kind.CLASS_DECLARATION)) {
         return element;
diff --git a/javascript-frontend/src/main/java/org/sonar/javascript/cfg/BranchingBlock.java b/javascript-frontend/src/main/java/org/sonar/javascript/cfg/BranchingBlock.java
@@ -46,7 +46,7 @@ public Set<MutableBlock> successors() {
 
   public void setSuccessors(MutableBlock successor1, MutableBlock successor2) {
     Preconditions.checkArgument(successor1 != null && successor2 != null, "Successor cannot be null");
-    Preconditions.checkArgument(successor1 != this && successor2 != this, "Cannot add itself as successor");
+    Preconditions.checkArgument(!this.equals(successor1) && !this.equals(successor2), "Cannot add itself as successor");
     this.successor1 = successor1;
     this.successor2 = successor2;
   }
diff --git a/javascript-frontend/src/main/java/org/sonar/javascript/cfg/ControlFlowGraphBuilder.java b/javascript-frontend/src/main/java/org/sonar/javascript/cfg/ControlFlowGraphBuilder.java
@@ -93,7 +93,7 @@ private void removeEmptyBlocks() {
     Map<MutableBlock, MutableBlock> emptyBlockReplacements = new HashMap<>();
     for (SingleSuccessorBlock block : Iterables.filter(blocks, SingleSuccessorBlock.class)) {
       if (block.isEmpty()) {
-        MutableBlock firstNonEmptySuccessor = block.firstNonEmptySuccessor();
+        MutableBlock firstNonEmptySuccessor = block.skipEmptyBlocks();
         emptyBlockReplacements.put(block, firstNonEmptySuccessor);
         for (SyntaxToken jump : block.disconnectingJumps()) {
           firstNonEmptySuccessor.addDisconnectingJump(jump);
diff --git a/javascript-frontend/src/main/java/org/sonar/javascript/cfg/MutableBlock.java b/javascript-frontend/src/main/java/org/sonar/javascript/cfg/MutableBlock.java
@@ -40,6 +40,11 @@ abstract class MutableBlock {
 
   public abstract Set<MutableBlock> successors();
   
+  /**
+   * Replace successors based on a replacement map.
+   * This method is used when we remove empty blocks:
+   * we have to replace empty successors in the remaining blocks by non-empty successors.
+   */
   public abstract void replaceSuccessors(Map<MutableBlock, MutableBlock> replacements);
 
   public List<Tree> elements() {
diff --git a/javascript-frontend/src/main/java/org/sonar/javascript/cfg/SingleSuccessorBlock.java b/javascript-frontend/src/main/java/org/sonar/javascript/cfg/SingleSuccessorBlock.java
@@ -38,8 +38,9 @@ public Set<MutableBlock> successors() {
     return ImmutableSet.of(successor());
   }
 
-  public MutableBlock firstNonEmptySuccessor() {
+  public MutableBlock skipEmptyBlocks() {
     MutableBlock block = this;
+    // BranchingBlock cannot be empty, EndBlock cannot be skipped.
     while (block instanceof SingleSuccessorBlock && block.isEmpty()) {
       block = ((SingleSuccessorBlock) block).successor();
     }
diff --git a/javascript-frontend/src/test/java/org/sonar/javascript/cfg/MutableBlockTest.java b/javascript-frontend/src/test/java/org/sonar/javascript/cfg/MutableBlockTest.java
@@ -83,11 +83,11 @@ public void cannot_add_null_as_successor() throws Exception {
   public void non_empty_successor() throws Exception {
     SimpleBlock simpleNonEmpty = new SimpleBlock(end);
     simpleNonEmpty.addElement(tree1);
-    assertThat(simpleNonEmpty.firstNonEmptySuccessor()).isEqualTo(simpleNonEmpty);
-    assertThat(new SimpleBlock(simpleNonEmpty).firstNonEmptySuccessor()).isEqualTo(simpleNonEmpty);
-    assertThat(new SimpleBlock(branching1).firstNonEmptySuccessor()).isEqualTo(branching1);
-    assertThat(new SimpleBlock(end).firstNonEmptySuccessor()).isEqualTo(end);
-    assertThat(new SimpleBlock(new SimpleBlock(end)).firstNonEmptySuccessor()).isEqualTo(end);
+    assertThat(simpleNonEmpty.skipEmptyBlocks()).isEqualTo(simpleNonEmpty);
+    assertThat(new SimpleBlock(simpleNonEmpty).skipEmptyBlocks()).isEqualTo(simpleNonEmpty);
+    assertThat(new SimpleBlock(branching1).skipEmptyBlocks()).isEqualTo(branching1);
+    assertThat(new SimpleBlock(end).skipEmptyBlocks()).isEqualTo(end);
+    assertThat(new SimpleBlock(new SimpleBlock(end)).skipEmptyBlocks()).isEqualTo(end);
   }
 
   @Test

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ private void check(ControlFlowGraph cfg) {`
`95`	`95`	`}`
`96`	`96`	`}`
`97`	`97`
`98`		`- private Tree unreachableTree(List<Tree> elements) {`
	`98`	`+ private static Tree unreachableTree(List<Tree> elements) {`
`99`	`99`	`for (Tree element : elements) {`
`100`	`100`	`if (!element.is(Kind.FUNCTION_DECLARATION, Kind.GENERATOR_DECLARATION, Kind.CLASS_DECLARATION)) {`
`101`	`101`	`return element;`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ public Set<MutableBlock> successors() {`
`46`	`46`
`47`	`47`	`public void setSuccessors(MutableBlock successor1, MutableBlock successor2) {`
`48`	`48`	`Preconditions.checkArgument(successor1 != null && successor2 != null, "Successor cannot be null");`
`49`		`- Preconditions.checkArgument(successor1 != this && successor2 != this, "Cannot add itself as successor");`
	`49`	`+ Preconditions.checkArgument(!this.equals(successor1) && !this.equals(successor2), "Cannot add itself as successor");`
`50`	`50`	`this.successor1 = successor1;`
`51`	`51`	`this.successor2 = successor2;`
`52`	`52`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,8 +38,9 @@ public Set<MutableBlock> successors() {`
`38`	`38`	`return ImmutableSet.of(successor());`
`39`	`39`	`}`
`40`	`40`
`41`		`- public MutableBlock firstNonEmptySuccessor() {`
	`41`	`+ public MutableBlock skipEmptyBlocks() {`
`42`	`42`	`MutableBlock block = this;`
	`43`	`+ // BranchingBlock cannot be empty, EndBlock cannot be skipped.`
`43`	`44`	`while (block instanceof SingleSuccessorBlock && block.isEmpty()) {`
`44`	`45`	`block = ((SingleSuccessorBlock) block).successor();`
`45`	`46`	`}`