[analyzer] Refactor recursive symbol reachability check to use symbol_iterator

[zaks.anna]

Thank you for the cleanup!!! For bonus points, please add comments to the class APIs:)

[NoQ]

lib/StaticAnalyzer/Core/ProgramState.cpp

542 ↗

(On Diff #78285)

I guess we should break the loop here.

[NoQ]

lib/StaticAnalyzer/Core/ProgramState.cpp

535 ↗

(On Diff #78285)

Hmm, the original code does actually visit non-SymbolData.

[ddcc]

Fix visitation, add early termination, add comments

[ddcc]

I believe you're correct, the original code terminates early because of the short circuit evaluation on line 553, and visits all reachable nodes but doesn't recurse on non-SymbolData.

[NoQ]

Yep, looks correct now :)

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h

 CB ProgramState::scanReachableSymbols(const MemRegion * const *beg,
                                  const MemRegion * const *end) const {
   CB cb(this);
   scanReachableSymbols(beg, end, cb);
   return cb;
 }
 
 /// \class ScanReachableSymbols
-/// A Utility class that allows to visit the reachable symbols using a custom
-/// SymbolVisitor.
+/// A utility class that visits the reachable symbols using a custom
+/// SymbolVisitor. Terminates recursive traversal when the visitor function
+/// returns false.
 class ScanReachableSymbols {
   typedef llvm::DenseSet<const void*> VisitedItems;
 
   VisitedItems visited;
   ProgramStateRef state;
   SymbolVisitor &visitor;
 public:
   ScanReachableSymbols(ProgramStateRef st, SymbolVisitor &v)

cfe/trunk/lib/StaticAnalyzer/Core/ProgramState.cpp

bool ScanReachableSymbols::scan(nonloc::CompoundVal val) {
   for (nonloc::CompoundVal::iterator I=val.begin(), E=val.end(); I!=E; ++I)
     if (!scan(*I))
       return false;
 
   return true;
 }
 
 bool ScanReachableSymbols::scan(const SymExpr *sym) {
-  bool wasVisited = !visited.insert(sym).second;
+  for (SymExpr::symbol_iterator SI = sym->symbol_begin(),
+                                SE = sym->symbol_end();
+       SI != SE; ++SI) {
+    bool wasVisited = !visited.insert(*SI).second;
     if (wasVisited)
-    return true;
+      continue;
 
-  if (!visitor.VisitSymbol(sym))
+    if (!visitor.VisitSymbol(*SI))
       return false;
-
-  // TODO: should be rewritten using SymExpr::symbol_iterator.
-  switch (sym->getKind()) {
-    case SymExpr::SymbolRegionValueKind:
-    case SymExpr::SymbolConjuredKind:
-    case SymExpr::SymbolDerivedKind:
-    case SymExpr::SymbolExtentKind:
-    case SymExpr::SymbolMetadataKind:
-      break;
-    case SymExpr::SymbolCastKind:
-      return scan(cast<SymbolCast>(sym)->getOperand());
-    case SymExpr::SymIntExprKind:
-      return scan(cast<SymIntExpr>(sym)->getLHS());
-    case SymExpr::IntSymExprKind:
-      return scan(cast<IntSymExpr>(sym)->getRHS());
-    case SymExpr::SymSymExprKind: {
-      const SymSymExpr *x = cast<SymSymExpr>(sym);
-      return scan(x->getLHS()) && scan(x->getRHS());
-    }
   }
+
   return true;
 }
 
 bool ScanReachableSymbols::scan(SVal val) {
   if (Optional<loc::MemRegionVal> X = val.getAs<loc::MemRegionVal>())
     return scan(X->getRegion());
 
   if (Optional<nonloc::LazyCompoundVal> X =