[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365179: [analyzer][Dominators][NFC] Add unit tests (authored 
by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62611?vs=207759=208134#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611

Files:
  cfe/trunk/unittests/Analysis/CFGBuildResult.h
  cfe/trunk/unittests/Analysis/CFGDominatorTree.cpp
  cfe/trunk/unittests/Analysis/CFGTest.cpp
  cfe/trunk/unittests/Analysis/CMakeLists.txt

Index: cfe/trunk/unittests/Analysis/CFGBuildResult.h
===
--- cfe/trunk/unittests/Analysis/CFGBuildResult.h
+++ cfe/trunk/unittests/Analysis/CFGBuildResult.h
@@ -0,0 +1,69 @@
+//===- unittests/Analysis/CFGBuildResult.h - CFG tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Analysis/CFG.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+
+namespace clang {
+namespace analysis {
+
+class BuildResult {
+public:
+  enum Status {
+ToolFailed,
+ToolRan,
+SawFunctionBody,
+BuiltCFG,
+  };
+
+  BuildResult(Status S, std::unique_ptr Cfg = nullptr)
+  : S(S), Cfg(std::move(Cfg)) {}
+
+  Status getStatus() const { return S; }
+  CFG *getCFG() const { return Cfg.get(); }
+
+private:
+  Status S;
+  std::unique_ptr Cfg;
+};
+
+class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
+public:
+  BuildResult TheBuildResult = BuildResult::ToolRan;
+
+  void run(const ast_matchers::MatchFinder::MatchResult ) override {
+const auto *Func = Result.Nodes.getNodeAs("func");
+Stmt *Body = Func->getBody();
+if (!Body)
+  return;
+TheBuildResult = BuildResult::SawFunctionBody;
+CFG::BuildOptions Options;
+Options.AddImplicitDtors = true;
+if (std::unique_ptr Cfg =
+CFG::buildCFG(nullptr, Body, Result.Context, Options))
+  TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
+  }
+};
+
+inline BuildResult BuildCFG(const char *Code) {
+  CFGCallback Callback;
+
+  ast_matchers::MatchFinder Finder;
+  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
+  std::unique_ptr Factory(
+  tooling::newFrontendActionFactory());
+  std::vector Args = {"-std=c++11",
+   "-fno-delayed-template-parsing"};
+  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
+return BuildResult::ToolFailed;
+  return std::move(Callback.TheBuildResult);
+}
+
+} // namespace analysis
+} // namespace clang
Index: cfe/trunk/unittests/Analysis/CFGTest.cpp
===
--- cfe/trunk/unittests/Analysis/CFGTest.cpp
+++ cfe/trunk/unittests/Analysis/CFGTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CFGBuildResult.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Tooling/Tooling.h"
@@ -17,57 +18,6 @@
 namespace analysis {
 namespace {
 
-class BuildResult {
-public:
-  enum Status {
-ToolFailed,
-ToolRan,
-SawFunctionBody,
-BuiltCFG,
-  };
-
-  BuildResult(Status S, std::unique_ptr Cfg = nullptr)
-  : S(S), Cfg(std::move(Cfg)) {}
-
-  Status getStatus() const { return S; }
-  CFG *getCFG() const { return Cfg.get(); }
-
-private:
-  Status S;
-  std::unique_ptr Cfg;
-};
-
-class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
-public:
-  BuildResult TheBuildResult = BuildResult::ToolRan;
-
-  void run(const ast_matchers::MatchFinder::MatchResult ) override {
-const auto *Func = Result.Nodes.getNodeAs("func");
-Stmt *Body = Func->getBody();
-if (!Body)
-  return;
-TheBuildResult = BuildResult::SawFunctionBody;
-CFG::BuildOptions Options;
-Options.AddImplicitDtors = true;
-if (std::unique_ptr Cfg =
-CFG::buildCFG(nullptr, Body, Result.Context, Options))
-  TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
-  }
-};
-
-BuildResult BuildCFG(const char *Code) {
-  CFGCallback Callback;
-
-  ast_matchers::MatchFinder Finder;
-  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
-  std::unique_ptr Factory(
-  tooling::newFrontendActionFactory());
-  std::vector Args = {"-std=c++11", "-fno-delayed-template-parsing"};
-  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
-return BuildResult::ToolFailed;
-  return std::move(Callback.TheBuildResult);
-}
-
 // Constructing a CFG for a 

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Looks good to me, some nits inline.




Comment at: clang/unittests/Analysis/CFGBuildResult.h:1
+//===- unittests/Analysis/CFGTest.cpp - CFG tests 
-===//
+//

Filename not updated?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:1
+//===- unittests/Analysis/CFGTest.cpp - CFG tests 
-===//
+//

Filename not updated?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:13
+#include "clang/Analysis/CFG.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"

Do you need the Tooling header here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 207759.
Szelethus added a comment.

Fixes according to reviewer comments!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611

Files:
  clang/unittests/Analysis/CFGBuildResult.h
  clang/unittests/Analysis/CFGDominatorTree.cpp
  clang/unittests/Analysis/CFGTest.cpp
  clang/unittests/Analysis/CMakeLists.txt

Index: clang/unittests/Analysis/CMakeLists.txt
===
--- clang/unittests/Analysis/CMakeLists.txt
+++ clang/unittests/Analysis/CMakeLists.txt
@@ -3,6 +3,7 @@
   )
 
 add_clang_unittest(ClangAnalysisTests
+  CFGDominatorTree.cpp
   CFGTest.cpp
   CloneDetectionTest.cpp
   ExprMutationAnalyzerTest.cpp
Index: clang/unittests/Analysis/CFGTest.cpp
===
--- clang/unittests/Analysis/CFGTest.cpp
+++ clang/unittests/Analysis/CFGTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CFGBuildResult.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Tooling/Tooling.h"
@@ -17,57 +18,6 @@
 namespace analysis {
 namespace {
 
-class BuildResult {
-public:
-  enum Status {
-ToolFailed,
-ToolRan,
-SawFunctionBody,
-BuiltCFG,
-  };
-
-  BuildResult(Status S, std::unique_ptr Cfg = nullptr)
-  : S(S), Cfg(std::move(Cfg)) {}
-
-  Status getStatus() const { return S; }
-  CFG *getCFG() const { return Cfg.get(); }
-
-private:
-  Status S;
-  std::unique_ptr Cfg;
-};
-
-class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
-public:
-  BuildResult TheBuildResult = BuildResult::ToolRan;
-
-  void run(const ast_matchers::MatchFinder::MatchResult ) override {
-const auto *Func = Result.Nodes.getNodeAs("func");
-Stmt *Body = Func->getBody();
-if (!Body)
-  return;
-TheBuildResult = BuildResult::SawFunctionBody;
-CFG::BuildOptions Options;
-Options.AddImplicitDtors = true;
-if (std::unique_ptr Cfg =
-CFG::buildCFG(nullptr, Body, Result.Context, Options))
-  TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
-  }
-};
-
-BuildResult BuildCFG(const char *Code) {
-  CFGCallback Callback;
-
-  ast_matchers::MatchFinder Finder;
-  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
-  std::unique_ptr Factory(
-  tooling::newFrontendActionFactory());
-  std::vector Args = {"-std=c++11", "-fno-delayed-template-parsing"};
-  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
-return BuildResult::ToolFailed;
-  return std::move(Callback.TheBuildResult);
-}
-
 // Constructing a CFG for a range-based for over a dependent type fails (but
 // should not crash).
 TEST(CFG, RangeBasedForOverDependentType) {
Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- /dev/null
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -0,0 +1,108 @@
+//===- unittests/Analysis/CFGTest.cpp - CFG tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CFGBuildResult.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/Dominators.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace analysis {
+namespace {
+
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = R"(enum Kind {
+  A
+};
+
+void f() {
+  switch(Kind{}) {
+  case A:
+break;
+  }
+})";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
+  //  switch  case A
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 4u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, >getExit());
+
+  CFGBlock *SwitchBlock = *(cfg->begin() + 1);
+
+  CFGBlock *CaseABlock = *(cfg->begin() + 2);
+
+  CFGBlock *EntryBlock = *(cfg->begin() + 3);
+  EXPECT_EQ(EntryBlock, >getEntry());
+
+  // Test the dominator tree.
+  CFGDomTree Dom;
+  Dom.buildDominatorTree(cfg);
+
+  EXPECT_TRUE(Dom.dominates(ExitBlock, ExitBlock));
+  EXPECT_FALSE(Dom.properlyDominates(ExitBlock, ExitBlock));
+  EXPECT_TRUE(Dom.dominates(CaseABlock, ExitBlock));
+  EXPECT_TRUE(Dom.dominates(SwitchBlock, ExitBlock));
+  EXPECT_TRUE(Dom.dominates(EntryBlock, ExitBlock));
+
+  

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:22-32
+template  struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};

NoQ wrote:
> Why isn't this a simple function template?
Hmmm, this entire thing is pointless, really. It is used as a sanity check of 
some sort, whether for example the 4th block really is what I believe it would 
be. But this is anything but a reliable way to test it.

I think this causes far more confusion than value, I'll just remove it, the 
actual tests are thorough enough enough to break if the CFG changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/unittests/Analysis/CFGBuilder.h:16
+
+class BuildResult {
+public:

Given that now it's in a header, i guess we need to pick a less generic name. 
Eg., `CFGBuildResult`.



Comment at: clang/unittests/Analysis/CFGBuilder.h:54
+
+inline BuildResult BuildCFG(const char *Code) {
+  CFGCallback Callback;

xazax.hun wrote:
> Do you expect the code to only contain one function? If so, you should 
> enforce it.
I guess it sucks that we can't re-use my `findDeclByName()` helper here.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:22-28
+template  struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};

Why isn't this a simple function template?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:30-32
+template  bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}

`StmtType` is unused. Did you mean `FindStmt`?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:31
+template  bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}

This looks like "`hasNoStmtType`" to me, did you mean `!=`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Gentle ping :^)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Sanaa82016 Najjar via Phabricator via cfe-commits]

sanaanajjar231288 added a comment.

In D62611#1521692 , @Szelethus wrote:

> Fixes according to reviewer comments, thanks! :)





CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ping.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Oh, I realized later that code I commented on were only moved from somewhere 
else. If you feel like tackling these comments feel free to do so in a separate 
patch so this one stays clean (no changes to moved code).




Comment at: clang/unittests/Analysis/CFGBuilder.h:18
+public:
+  enum Status {
+ToolFailed,

Is it actually sensible to write tests where the tool failed? I can imagine 
these status codes being helpful for debugging but they has little to do with 
the tests. I wonder if we actually need all these or a nullable pointer as a 
result is sufficient.



Comment at: clang/unittests/Analysis/CFGBuilder.h:54
+
+inline BuildResult BuildCFG(const char *Code) {
+  CFGCallback Callback;

Do you expect the code to only contain one function? If so, you should enforce 
it.



Comment at: clang/unittests/Analysis/CFGBuilder.h:65
+return BuildResult::ToolFailed;
+  return std::move(Callback.TheBuildResult);
+}

Do you need the std::move here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 202004.
Szelethus added a comment.

Fixes according to reviewer comments, thanks! :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611

Files:
  clang/unittests/Analysis/CFGBuilder.h
  clang/unittests/Analysis/CFGDominatorTree.cpp
  clang/unittests/Analysis/CFGTest.cpp
  clang/unittests/Analysis/CMakeLists.txt

Index: clang/unittests/Analysis/CMakeLists.txt
===
--- clang/unittests/Analysis/CMakeLists.txt
+++ clang/unittests/Analysis/CMakeLists.txt
@@ -3,6 +3,7 @@
   )
 
 add_clang_unittest(ClangAnalysisTests
+  CFGDominatorTree.cpp
   CFGTest.cpp
   CloneDetectionTest.cpp
   ExprMutationAnalyzerTest.cpp
Index: clang/unittests/Analysis/CFGTest.cpp
===
--- clang/unittests/Analysis/CFGTest.cpp
+++ clang/unittests/Analysis/CFGTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CFGBuilder.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Tooling/Tooling.h"
@@ -17,57 +18,6 @@
 namespace analysis {
 namespace {
 
-class BuildResult {
-public:
-  enum Status {
-ToolFailed,
-ToolRan,
-SawFunctionBody,
-BuiltCFG,
-  };
-
-  BuildResult(Status S, std::unique_ptr Cfg = nullptr)
-  : S(S), Cfg(std::move(Cfg)) {}
-
-  Status getStatus() const { return S; }
-  CFG *getCFG() const { return Cfg.get(); }
-
-private:
-  Status S;
-  std::unique_ptr Cfg;
-};
-
-class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
-public:
-  BuildResult TheBuildResult = BuildResult::ToolRan;
-
-  void run(const ast_matchers::MatchFinder::MatchResult ) override {
-const auto *Func = Result.Nodes.getNodeAs("func");
-Stmt *Body = Func->getBody();
-if (!Body)
-  return;
-TheBuildResult = BuildResult::SawFunctionBody;
-CFG::BuildOptions Options;
-Options.AddImplicitDtors = true;
-if (std::unique_ptr Cfg =
-CFG::buildCFG(nullptr, Body, Result.Context, Options))
-  TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
-  }
-};
-
-BuildResult BuildCFG(const char *Code) {
-  CFGCallback Callback;
-
-  ast_matchers::MatchFinder Finder;
-  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
-  std::unique_ptr Factory(
-  tooling::newFrontendActionFactory());
-  std::vector Args = {"-std=c++11", "-fno-delayed-template-parsing"};
-  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
-return BuildResult::ToolFailed;
-  return std::move(Callback.TheBuildResult);
-}
-
 // Constructing a CFG for a range-based for over a dependent type fails (but
 // should not crash).
 TEST(CFG, RangeBasedForOverDependentType) {
Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- /dev/null
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -0,0 +1,122 @@
+//===- unittests/Analysis/CFGTest.cpp - CFG tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CFGBuilder.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/Dominators.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace analysis {
+namespace {
+
+template  struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};
+
+template  bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}
+
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = R"(enum Kind {
+  A
+};
+
+void f() {
+  switch(Kind{}) {
+  case A:
+break;
+  }
+})";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
+  //  switch  case A
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 4u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, >getExit());
+
+  CFGBlock *SwitchBlock = *(cfg->begin() + 1);
+  EXPECT_TRUE(hasStmtType(SwitchBlock));
+
+  CFGBlock *CaseABlock = *(cfg->begin() + 2);
+  EXPECT_TRUE(hasStmtType(CaseABlock));
+
+  CFGBlock *EntryBlock = *(cfg->begin() + 3);
+  EXPECT_EQ(EntryBlock, >getEntry());
+
+  // Test 

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked 5 inline comments as done.
Szelethus added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:95
+  CFGPostDomTree PostDom;
+  PostDom.buildDominatorTree(cfg);
+

kuhar wrote:
> Why not have a constructor that takes the cfg and constructs a domtree 
> straight away? But this should probably go into a separate patch.
Yea, it's been bothering me for a while, but apparently not enough to fix it 
(just yet!).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:37
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = "enum Kind {\n"
+ "  A\n"

kuhar wrote:
> You can use a raw string literal to make it more readable: 
> https://en.cppreference.com/w/cpp/language/string_literal
...or just omit `\n`s because the compiler doesn't really care.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:95
+  CFGPostDomTree PostDom;
+  PostDom.buildDominatorTree(cfg);
+

Why not have a constructor that takes the cfg and constructs a domtree straight 
away? But this should probably go into a separate patch.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:113
+  EXPECT_TRUE(PostDom.dominates(ExitBlock, ExitBlock));
+  EXPECT_FALSE(Dom.properlyDominates(ExitBlock, ExitBlock));
+}

A tastcase with a virtual root postdominating other nodes would be welcome.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:37
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = "enum Kind {\n"
+ "  A\n"

You can use a raw string literal to make it more readable: 
https://en.cppreference.com/w/cpp/language/string_literal


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62611/new/

https://reviews.llvm.org/D62611



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, baloghadamsoftware, 
Charusso, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, kuhar, whisperity, mgorny.

Exactly what it says on the tin!


Repository:
  rC Clang

https://reviews.llvm.org/D62611

Files:
  clang/unittests/Analysis/CFGBuilder.h
  clang/unittests/Analysis/CFGDominatorTree.cpp
  clang/unittests/Analysis/CFGTest.cpp
  clang/unittests/Analysis/CMakeLists.txt

Index: clang/unittests/Analysis/CMakeLists.txt
===
--- clang/unittests/Analysis/CMakeLists.txt
+++ clang/unittests/Analysis/CMakeLists.txt
@@ -3,6 +3,7 @@
   )
 
 add_clang_unittest(ClangAnalysisTests
+  CFGDominatorTree.cpp
   CFGTest.cpp
   CloneDetectionTest.cpp
   ExprMutationAnalyzerTest.cpp
Index: clang/unittests/Analysis/CFGTest.cpp
===
--- clang/unittests/Analysis/CFGTest.cpp
+++ clang/unittests/Analysis/CFGTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CFGBuilder.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Tooling/Tooling.h"
@@ -17,57 +18,6 @@
 namespace analysis {
 namespace {
 
-class BuildResult {
-public:
-  enum Status {
-ToolFailed,
-ToolRan,
-SawFunctionBody,
-BuiltCFG,
-  };
-
-  BuildResult(Status S, std::unique_ptr Cfg = nullptr)
-  : S(S), Cfg(std::move(Cfg)) {}
-
-  Status getStatus() const { return S; }
-  CFG *getCFG() const { return Cfg.get(); }
-
-private:
-  Status S;
-  std::unique_ptr Cfg;
-};
-
-class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
-public:
-  BuildResult TheBuildResult = BuildResult::ToolRan;
-
-  void run(const ast_matchers::MatchFinder::MatchResult ) override {
-const auto *Func = Result.Nodes.getNodeAs("func");
-Stmt *Body = Func->getBody();
-if (!Body)
-  return;
-TheBuildResult = BuildResult::SawFunctionBody;
-CFG::BuildOptions Options;
-Options.AddImplicitDtors = true;
-if (std::unique_ptr Cfg =
-CFG::buildCFG(nullptr, Body, Result.Context, Options))
-  TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
-  }
-};
-
-BuildResult BuildCFG(const char *Code) {
-  CFGCallback Callback;
-
-  ast_matchers::MatchFinder Finder;
-  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
-  std::unique_ptr Factory(
-  tooling::newFrontendActionFactory());
-  std::vector Args = {"-std=c++11", "-fno-delayed-template-parsing"};
-  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
-return BuildResult::ToolFailed;
-  return std::move(Callback.TheBuildResult);
-}
-
 // Constructing a CFG for a range-based for over a dependent type fails (but
 // should not crash).
 TEST(CFG, RangeBasedForOverDependentType) {
Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- /dev/null
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -0,0 +1,118 @@
+//===- unittests/Analysis/CFGTest.cpp - CFG tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CFGBuilder.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/Dominators.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+
+namespace clang {
+namespace analysis {
+namespace {
+
+template 
+struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};
+
+template 
+bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}
+
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = "enum Kind {\n"
+ "  A\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  switch(Kind{}) {\n"
+ "  case A:\n"
+ "break;\n"
+ "  }\n"
+ "}\n";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
+  //  switch  case A
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 4u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, >getExit());
+
+  CFGBlock *SwitchBlock = *(cfg->begin() + 1);
+