Changes from code review

google · Feb 24, 2012 · eeef918 · eeef918
1 parent 1df93af
commit eeef918
Showing 1 changed file with 73 additions and 53 deletions.
diff --git a/core/src/main/java/com/google/errorprone/Scanner.java b/core/src/main/java/com/google/errorprone/Scanner.java
@@ -41,57 +41,39 @@
  * @author alexeagle@google.com (Alex Eagle)
  */
 public class Scanner extends TreePathScanner<Void, VisitorState> {
-  
+
   private Set<String> suppressions = new HashSet<String>(); 
-
-   @Override
-  public Void scan(TreePath path, VisitorState state) {
-     Set<String> newSuppressions = null;
-     Set<String> prevSuppressions = suppressions;
-     Symbol sym = getSymbol(path.getLeaf());
-     if (sym != null) {
-       newSuppressions = extendSuppressionSet(sym, state.getSymtab().suppressWarningsType, 
-           suppressions);
-       if (newSuppressions != null) {
-         suppressions = newSuppressions;
-       }
-     }
-
-     try {
-       return super.scan(path, state);
-     } finally {
-       suppressions = prevSuppressions;
-     }
-   }
 
   /**
-   * Given an AST node, returns its symbol.  Only certain AST nodes have
-   * symbols, so if there is no symbol for this node, returns null.
-   * 
-   * @param tree The AST node for which to get the symbol
-   * @return The symbol if it exists, null otherwise
+   * Scan a tree from a position identified by a TreePath. 
    */
-  private Symbol getSymbol(Tree tree) {
-    Symbol sym = null;
-    switch (tree.getKind()) {
-      case CLASS:
-        sym = ((JCClassDecl) tree).sym;
-        break;
-      case METHOD:
-        sym = ((JCMethodDecl) tree).sym; 
-        break;
-      case VARIABLE:
-        sym = ((JCVariableDecl) tree).sym;
-        break;
-      case MEMBER_SELECT:
-        sym = ((JCFieldAccess) tree).sym;
-        break;
-      case IDENTIFIER:
-        sym = ((JCIdent) tree).sym;
-        break;
+  @Override
+  public Void scan(TreePath path, VisitorState state) {
+    /**
+     * We maintain a list of suppressed warnings for the current node. When we
+     * explore a new node, we have to extend the suppression set with any new
+     * suppressed warnings.  We also have to retain the previous suppression set
+     * so that we can reinstate it when we move up the tree.
+     * 
+     * We avoid copying the suppression set if the next node to explore does not
+     * have any suppressed warnings.  This is the common case. 
+     */
+    Set<String> newSuppressions = null;
+    Set<String> prevSuppressions = suppressions;
+    Symbol sym = getSymbol(path.getLeaf());
+    if (sym != null) {
+      newSuppressions = extendSuppressionSet(sym, state.getSymtab().suppressWarningsType, 
+          suppressions);
+      if (newSuppressions != null) {
+        suppressions = newSuppressions;
+      }
+    }
+
+    try {
+      return super.scan(path, state);
+    } finally {
+      suppressions = prevSuppressions;
     }
-
-    return sym;
   }
 
   /**
@@ -103,7 +85,16 @@ public Void scan(Tree tree, VisitorState state) {
     if (tree == null) {
       return null;
     }
-
+
+    /**
+     * We maintain a list of suppressed warnings for the current node. When we
+     * explore a new node, we have to extend the suppression set with any new
+     * suppressed warnings.  We also have to retain the previous suppression set
+     * so that we can reinstate it when we move up the tree.
+     * 
+     * We avoid copying the suppression set if the next node to explore does not
+     * have any suppressed warnings.  This is the common case. 
+     */
     Set<String> newSuppressions = null;
     Set<String> prevSuppressions = suppressions;
     Symbol sym = getSymbol(tree);
@@ -114,15 +105,45 @@ public Void scan(Tree tree, VisitorState state) {
         suppressions = newSuppressions;
       }
     }
-    
+
     try {    
       return super.scan(tree, state);
     } finally {
       suppressions = prevSuppressions;
     }
+
+  }
 
+  /**
+   * Given an AST node, returns its symbol.  Only certain AST nodes have
+   * symbols, so if there is no symbol for this node, returns null.
+   * 
+   * @param tree The AST node for which to get the symbol
+   * @return The symbol if it exists, null otherwise
+   */
+  private Symbol getSymbol(Tree tree) {
+    Symbol sym = null;
+    switch (tree.getKind()) {
+      case CLASS:
+        sym = ((JCClassDecl) tree).sym;
+        break;
+      case METHOD:
+        sym = ((JCMethodDecl) tree).sym; 
+        break;
+      case VARIABLE:
+        sym = ((JCVariableDecl) tree).sym;
+        break;
+      case MEMBER_SELECT:
+        sym = ((JCFieldAccess) tree).sym;
+        break;
+      case IDENTIFIER:
+        sym = ((JCIdent) tree).sym;
+        break;
+    }
+
+    return sym;
   }
-  
+
   /**
    * Extends a set of suppressed warnings with the contents of any SuppressWarnings annotations
    * on the given symbol.  Does not mutate the passed-in set of suppressions.  If there were
@@ -138,7 +159,7 @@ private Set<String> extendSuppressionSet(Symbol sym, Type suppressWarningsType,
       Set<String> suppressions) {
     boolean copied = false;
     Set<String> newSuppressions = null;
-    
+
     // Iterate over annotations on this symbol, looking for SuppressWarnings
     for (Attribute.Compound attr : sym.getAnnotationMirrors()) {
       if (attr.type.tsym == suppressWarningsType.tsym) {
@@ -161,15 +182,14 @@ private Set<String> extendSuppressionSet(Symbol sym, Type suppressWarningsType,
         }
       }
     }
-    
+
     return newSuppressions;
   }
-  
+
   public boolean isSuppressed(String warning) {
     return suppressions.contains(warning);
   }
 
-
   protected <T extends Tree> void reportMatch(Matcher<T> matcher, T match, VisitorState state) {
     state.getMatchListener().onMatch(match);
     if (matcher instanceof RefactoringMatcher) {