Skip to content

[analyzer][NFC] Remove check::BranchCondition from debug.DumpTraversal #113906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 7, 2024
Merged

[analyzer][NFC] Remove check::BranchCondition from debug.DumpTraversal #113906

merged 2 commits into from
Nov 7, 2024

Conversation

NagyDonat
Copy link
Contributor

This commit removes the check::BranchCondition callback of the debug checker debug.DumpTraversal (in TraversalChecker.cpp) and the single broken testcase that was referring to it.

The testcase traversal-algorithm.mm was added in 2012 to verify that we're using DFS traversal -- however it failed to detect that we're no longer using DFS traversal and in fact it continues to pass even if I remove large random portions of its code.

This change is motivated by the plan discussed at
https://discourse.llvm.org/t/fixing-or-removing-check-branchcondition/82738

I also added some TODO notes to mark the rest of TraversalChecker.cpp for removal in follow-up commits.

@NagyDonat
[analyzer][NFC] Remove check::BranchCondition from debug.DumpTraversal
003236d 
This commit removes the `check::BranchCondition` callback of the debug
checker `debug.DumpTraversal` (in `TraversalChecker.cpp`) and the single
broken testcase that was referring to it.

The testcase `traversal-algorithm.mm` was added in 2012 to verify that
we're using DFS traversal -- however it failed to detect that we're no
longer using DFS traversal and in fact it continues to pass even if I
remove large random portions of its code.

This change is motivated by the plan discussed at
https://discourse.llvm.org/t/fixing-or-removing-check-branchcondition/82738

I also added some TODO notes to mark the rest of `TraversalChecker.cpp`
for removal in follow-up commits.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Oct 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2024

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

This commit removes the check::BranchCondition callback of the debug checker debug.DumpTraversal (in TraversalChecker.cpp) and the single broken testcase that was referring to it.

The testcase traversal-algorithm.mm was added in 2012 to verify that we're using DFS traversal -- however it failed to detect that we're no longer using DFS traversal and in fact it continues to pass even if I remove large random portions of its code.

This change is motivated by the plan discussed at
https://discourse.llvm.org/t/fixing-or-removing-check-branchcondition/82738

I also added some TODO notes to mark the rest of TraversalChecker.cpp for removal in follow-up commits.

Full diff: https://github.com/llvm/llvm-project/pull/113906.diff

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp (+7-21)
  • (removed) clang/test/Analysis/traversal-algorithm.mm (-213)
diff --git a/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
index 2f316bd3b20dbe..9027fa3f4dba8e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
@@ -23,34 +23,17 @@ using namespace clang;
 using namespace ento;
 
 namespace {
-class TraversalDumper : public Checker< check::BranchCondition,
-                                        check::BeginFunction,
+// TODO: This checker is only referenced from two small test files and it
+// doesn't seem to be useful for manual debugging, so consider reimplementing
+// those tests with more modern tools and removing this checker.
+class TraversalDumper : public Checker< check::BeginFunction,
                                         check::EndFunction > {
 public:
-  void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const;
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
 };
 }
 
-void TraversalDumper::checkBranchCondition(const Stmt *Condition,
-                                           CheckerContext &C) const {
-  // Special-case Objective-C's for-in loop, which uses the entire loop as its
-  // condition. We just print the collection expression.
-  const Stmt *Parent = dyn_cast<ObjCForCollectionStmt>(Condition);
-  if (!Parent) {
-    const ParentMap &Parents = C.getLocationContext()->getParentMap();
-    Parent = Parents.getParent(Condition);
-  }
-
-  // It is mildly evil to print directly to llvm::outs() rather than emitting
-  // warnings, but this ensures things do not get filtered out by the rest of
-  // the static analyzer machinery.
-  SourceLocation Loc = Parent->getBeginLoc();
-  llvm::outs() << C.getSourceManager().getSpellingLineNumber(Loc) << " "
-               << Parent->getStmtClassName() << "\n";
-}
-
 void TraversalDumper::checkBeginFunction(CheckerContext &C) const {
   llvm::outs() << "--BEGIN FUNCTION--\n";
 }
@@ -71,6 +54,9 @@ bool ento::shouldRegisterTraversalDumper(const CheckerManager &mgr) {
 //------------------------------------------------------------------------------
 
 namespace {
+// TODO: This checker appears to be a utility for creating `FileCheck` tests
+// verifying its stdout output, but there are no tests that rely on it, so
+// perhaps it should be removed.
 class CallDumper : public Checker< check::PreCall,
                                    check::PostCall > {
 public:
diff --git a/clang/test/Analysis/traversal-algorithm.mm b/clang/test/Analysis/traversal-algorithm.mm
deleted file mode 100644
index bdf576063d656c..00000000000000
--- a/clang/test/Analysis/traversal-algorithm.mm
+++ /dev/null
@@ -1,213 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpTraversal -analyzer-max-loop 4 -std=c++11 %s | FileCheck -check-prefix=DFS %s
-
-int a();
-int b();
-int c();
-
-int work();
-
-void test(id input) {
-  if (a()) {
-    if (a())
-      b();
-    else
-      c();
-  } else {
-    if (b())
-      a();
-    else
-      c();
-  }
-
-  if (a())
-    work();
-}
-
-void testLoops(id input) {
-  while (a()) {
-    work();
-    work();
-    work();
-  }
-
-  for (int i = 0; i != b(); ++i) {
-    work();
-  }
-
-  for (id x in input) {
-    work();
-    work();
-    work();
-  }
-
-  int z[] = {1,2,3};
-  for (int y : z) {
-    work();
-    work();
-    work();
-  }
-}
-
-// This ordering assumes that false cases happen before the true cases.
-
-// DFS:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:10 IfStmt
-// DFS-next:16 IfStmt
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:11 IfStmt
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-

@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2024

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

This commit removes the check::BranchCondition callback of the debug checker debug.DumpTraversal (in TraversalChecker.cpp) and the single broken testcase that was referring to it.

The testcase traversal-algorithm.mm was added in 2012 to verify that we're using DFS traversal -- however it failed to detect that we're no longer using DFS traversal and in fact it continues to pass even if I remove large random portions of its code.

This change is motivated by the plan discussed at
https://discourse.llvm.org/t/fixing-or-removing-check-branchcondition/82738

I also added some TODO notes to mark the rest of TraversalChecker.cpp for removal in follow-up commits.

Full diff: https://github.com/llvm/llvm-project/pull/113906.diff

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp (+7-21)
  • (removed) clang/test/Analysis/traversal-algorithm.mm (-213)
diff --git a/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
index 2f316bd3b20dbe..9027fa3f4dba8e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
@@ -23,34 +23,17 @@ using namespace clang;
 using namespace ento;
 
 namespace {
-class TraversalDumper : public Checker< check::BranchCondition,
-                                        check::BeginFunction,
+// TODO: This checker is only referenced from two small test files and it
+// doesn't seem to be useful for manual debugging, so consider reimplementing
+// those tests with more modern tools and removing this checker.
+class TraversalDumper : public Checker< check::BeginFunction,
                                         check::EndFunction > {
 public:
-  void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const;
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
 };
 }
 
-void TraversalDumper::checkBranchCondition(const Stmt *Condition,
-                                           CheckerContext &C) const {
-  // Special-case Objective-C's for-in loop, which uses the entire loop as its
-  // condition. We just print the collection expression.
-  const Stmt *Parent = dyn_cast<ObjCForCollectionStmt>(Condition);
-  if (!Parent) {
-    const ParentMap &Parents = C.getLocationContext()->getParentMap();
-    Parent = Parents.getParent(Condition);
-  }
-
-  // It is mildly evil to print directly to llvm::outs() rather than emitting
-  // warnings, but this ensures things do not get filtered out by the rest of
-  // the static analyzer machinery.
-  SourceLocation Loc = Parent->getBeginLoc();
-  llvm::outs() << C.getSourceManager().getSpellingLineNumber(Loc) << " "
-               << Parent->getStmtClassName() << "\n";
-}
-
 void TraversalDumper::checkBeginFunction(CheckerContext &C) const {
   llvm::outs() << "--BEGIN FUNCTION--\n";
 }
@@ -71,6 +54,9 @@ bool ento::shouldRegisterTraversalDumper(const CheckerManager &mgr) {
 //------------------------------------------------------------------------------
 
 namespace {
+// TODO: This checker appears to be a utility for creating `FileCheck` tests
+// verifying its stdout output, but there are no tests that rely on it, so
+// perhaps it should be removed.
 class CallDumper : public Checker< check::PreCall,
                                    check::PostCall > {
 public:
diff --git a/clang/test/Analysis/traversal-algorithm.mm b/clang/test/Analysis/traversal-algorithm.mm
deleted file mode 100644
index bdf576063d656c..00000000000000
--- a/clang/test/Analysis/traversal-algorithm.mm
+++ /dev/null
@@ -1,213 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpTraversal -analyzer-max-loop 4 -std=c++11 %s | FileCheck -check-prefix=DFS %s
-
-int a();
-int b();
-int c();
-
-int work();
-
-void test(id input) {
-  if (a()) {
-    if (a())
-      b();
-    else
-      c();
-  } else {
-    if (b())
-      a();
-    else
-      c();
-  }
-
-  if (a())
-    work();
-}
-
-void testLoops(id input) {
-  while (a()) {
-    work();
-    work();
-    work();
-  }
-
-  for (int i = 0; i != b(); ++i) {
-    work();
-  }
-
-  for (id x in input) {
-    work();
-    work();
-    work();
-  }
-
-  int z[] = {1,2,3};
-  for (int y : z) {
-    work();
-    work();
-    work();
-  }
-}
-
-// This ordering assumes that false cases happen before the true cases.
-
-// DFS:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:27 WhileStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:33 ForStmt
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:44 CXXForRangeStmt
-// DFS-next:--END PATH--
-// DFS-next:37 ObjCForCollectionStmt
-// DFS-next:10 IfStmt
-// DFS-next:16 IfStmt
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:11 IfStmt
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-// DFS-next:22 IfStmt
-// DFS-next:--END PATH--
-// DFS-next:--END PATH--
-

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 28, 2024 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@NagyDonat
Copy link
Contributor Author

This PR takes a conservative approach and only removes the BranchCondition callback from TraversalChecker.cpp.

However I don't think that TraversalChecker.cpp is useful for manual debugging and it's only used in two very simple testcases [1] [2] so perhaps it would be better to completely remove it.

What do you think about these follow-up removals? Do you see any purpose that makes it worth to keep TraversalChecker.cpp and the two testcases that rely on it?

steakhal
steakhal approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are fundamental problems with using stdout and FileCheck. Those assertions (CHECK line) depend on their relative ordering. However, the ordering of the top-level entry points may change by design. This made it impractical in the past to use FileCheck for tests, and was generally avoided.

@NagyDonat
Copy link
Contributor Author

Thanks for the information!

I'll merge this now and I might create a followup commit to remove the rest of TraversalChecker.cpp later.

@NagyDonat NagyDonat merged commit e40a31b into llvm:main Nov 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steakhal steakhal steakhal approved these changes

@Xazax-hun Xazax-hun Awaiting requested review from Xazax-hun

Assignees
No one assigned
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NagyDonat @llvmbot @steakhal