[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-20 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL311280: [clang-diff] Filter AST nodes (authored by krobelus).

Changed prior to commit:
  https://reviews.llvm.org/D36184?vs=110687=111872#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36184

Files:
  cfe/trunk/lib/Tooling/ASTDiff/ASTDiff.cpp
  cfe/trunk/test/Tooling/clang-diff-ast.cpp
  cfe/trunk/test/Tooling/clang-diff-json.cpp

Index: cfe/trunk/lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- cfe/trunk/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ cfe/trunk/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -158,12 +158,23 @@
   void setLeftMostDescendants();
 };
 
+static bool isSpecializedNodeExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isSpecializedNodeExcluded(const Stmt *S) { return false; }
+
 template 
 static bool isNodeExcluded(const SourceManager , T *N) {
   if (!N)
 return true;
   SourceLocation SLoc = N->getLocStart();
-  return SLoc.isValid() && SrcMgr.isInSystemHeader(SLoc);
+  if (SLoc.isValid()) {
+// Ignore everything from other files.
+if (!SrcMgr.isInMainFile(SLoc))
+  return true;
+// Ignore macros.
+if (SLoc != SrcMgr.getSpellingLoc(SLoc))
+  return true;
+  }
+  return isSpecializedNodeExcluded(N);
 }
 
 namespace {
@@ -180,6 +191,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 ++Count;
@@ -242,6 +255,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 auto SavedState = PreTraverse(S);
Index: cfe/trunk/test/Tooling/clang-diff-ast.cpp
===
--- cfe/trunk/test/Tooling/clang-diff-ast.cpp
+++ cfe/trunk/test/Tooling/clang-diff-ast.cpp
@@ -12,7 +12,8 @@
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: CallExpr(
-  // CHECK: DeclRefExpr: f(
+  // CHECK-NOT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr: f(
   f();
   // CHECK: BinaryOperator: =(
   i = i;
@@ -37,6 +38,7 @@
 if (i == 0)
   // CHECK: StringLiteral: foo(
   return "foo";
+// CHECK-NOT: ImplicitCastExpr
 return 0;
   }
 
@@ -48,3 +50,23 @@
 int x = m;
   }
 };
+
+#define M (void)1
+#define MA(a, b) (void)a, b
+// CHECK: FunctionDecl
+// CHECK-NEXT: CompoundStmt
+void macros() {
+  M;
+  MA(1, 2);
+}
+
+#ifndef GUARD
+#define GUARD
+// CHECK-NEXT: NamespaceDecl
+namespace world {
+// nodes from other files are excluded, there should be no output here
+#include "clang-diff-ast.cpp"
+}
+// CHECK-NEXT: FunctionDecl: sentinel
+void sentinel();
+#endif
Index: cfe/trunk/test/Tooling/clang-diff-json.cpp
===
--- cfe/trunk/test/Tooling/clang-diff-json.cpp
+++ cfe/trunk/test/Tooling/clang-diff-json.cpp
@@ -3,9 +3,9 @@
 // RUN: | FileCheck %s
 
 // CHECK: "begin": 299,
-// CHECK: "type": "CXXRecordDecl",
 // CHECK: "type": "FieldDecl",
 // CHECK: "end": 319,
+// CHECK: "type": "CXXRecordDecl",
 class A {
   int x;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-11 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: test/Tooling/clang-diff-ast.cpp:68
+// nodes from other files are excluded
+// CHECK-NOT {{.}}
+#include "clang-diff-ast.cpp"

johannes wrote:
> arphaman wrote:
> > 1) Missing ':'
> > 2) What exactly does this regex accomplish? Right now it will match any 
> > character which doesn't look correct
> I want to assert that there is no output here, because other files are 
> excluded, there may be a better way..
Ok, fair enough.


https://reviews.llvm.org/D36184



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

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-11 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes updated this revision to Diff 110687.
johannes added a comment.

clarify test/Tooling/clang-diff-ast.cpp


https://reviews.llvm.org/D36184

Files:
  lib/Tooling/ASTDiff/ASTDiff.cpp
  test/Tooling/clang-diff-ast.cpp
  test/Tooling/clang-diff-json.cpp

Index: test/Tooling/clang-diff-json.cpp
===
--- test/Tooling/clang-diff-json.cpp
+++ test/Tooling/clang-diff-json.cpp
@@ -3,9 +3,9 @@
 // RUN: | FileCheck %s
 
 // CHECK: "begin": 299,
-// CHECK: "type": "CXXRecordDecl",
 // CHECK: "type": "FieldDecl",
 // CHECK: "end": 319,
+// CHECK: "type": "CXXRecordDecl",
 class A {
   int x;
 };
Index: test/Tooling/clang-diff-ast.cpp
===
--- test/Tooling/clang-diff-ast.cpp
+++ test/Tooling/clang-diff-ast.cpp
@@ -12,7 +12,8 @@
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: CallExpr(
-  // CHECK: DeclRefExpr: f(
+  // CHECK-NOT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr: f(
   f();
   // CHECK: BinaryOperator: =(
   i = i;
@@ -37,6 +38,7 @@
 if (i == 0)
   // CHECK: StringLiteral: foo(
   return "foo";
+// CHECK-NOT: ImplicitCastExpr
 return 0;
   }
 
@@ -48,3 +50,23 @@
 int x = m;
   }
 };
+
+#define M (void)1
+#define MA(a, b) (void)a, b
+// CHECK: FunctionDecl
+// CHECK-NEXT: CompoundStmt
+void macros() {
+  M;
+  MA(1, 2);
+}
+// CHECK-NEXT: NamespaceDecl
+
+#ifndef GUARD
+#define GUARD
+namespace world {
+// nodes from other files are excluded, there should be no output from this
+// point on
+// CHECK-NOT: {{.}}
+#include "clang-diff-ast.cpp"
+}
+#endif
Index: lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- lib/Tooling/ASTDiff/ASTDiff.cpp
+++ lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -68,7 +68,8 @@
   // Compute ChangeKind for each node based on similarity.
   void computeChangeKinds(Mapping );
 
-  NodeId getMapped(const std::unique_ptr , NodeId Id) const {
+  NodeId getMapped(const std::unique_ptr ,
+   NodeId Id) const {
 if (&*Tree == )
   return TheMapping.getDst(Id);
 assert(&*Tree ==  && "Invalid tree.");
@@ -157,12 +158,23 @@
   void setLeftMostDescendants();
 };
 
+static bool isSpecializedNodeExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isSpecializedNodeExcluded(const Stmt *S) { return false; }
+
 template 
 static bool isNodeExcluded(const SourceManager , T *N) {
   if (!N)
 return true;
   SourceLocation SLoc = N->getLocStart();
-  return SLoc.isValid() && SrcMgr.isInSystemHeader(SLoc);
+  if (SLoc.isValid()) {
+// Ignore everything from other files.
+if (!SrcMgr.isInMainFile(SLoc))
+  return true;
+// Ignore macros.
+if (SLoc != SrcMgr.getSpellingLoc(SLoc))
+  return true;
+  }
+  return isSpecializedNodeExcluded(N);
 }
 
 namespace {
@@ -179,6 +191,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 ++Count;
@@ -241,6 +255,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 auto SavedState = PreTraverse(S);
@@ -900,7 +916,8 @@
   return TreeImpl->findPositionInParent(Id);
 }
 
-std::pair SyntaxTree::getSourceRangeOffsets(const Node ) const {
+std::pair
+SyntaxTree::getSourceRangeOffsets(const Node ) const {
   const SourceManager  = TreeImpl->AST.getSourceManager();
   SourceRange Range = N.ASTNode.getSourceRange();
   SourceLocation BeginLoc = Range.getBegin();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-10 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes added inline comments.



Comment at: test/Tooling/clang-diff-ast.cpp:68
+// nodes from other files are excluded
+// CHECK-NOT {{.}}
+#include "clang-diff-ast.cpp"

arphaman wrote:
> 1) Missing ':'
> 2) What exactly does this regex accomplish? Right now it will match any 
> character which doesn't look correct
I want to assert that there is no output here, because other files are 
excluded, there may be a better way..


https://reviews.llvm.org/D36184



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

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-10 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: test/Tooling/clang-diff-ast.cpp:68
+// nodes from other files are excluded
+// CHECK-NOT {{.}}
+#include "clang-diff-ast.cpp"

1) Missing ':'
2) What exactly does this regex accomplish? Right now it will match any 
character which doesn't look correct


https://reviews.llvm.org/D36184



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

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-10 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes updated this revision to Diff 110555.
johannes added a comment.

refactor isNodeExcluded


https://reviews.llvm.org/D36184

Files:
  lib/Tooling/ASTDiff/ASTDiff.cpp
  test/Tooling/clang-diff-ast.cpp
  test/Tooling/clang-diff-json.cpp

Index: test/Tooling/clang-diff-json.cpp
===
--- test/Tooling/clang-diff-json.cpp
+++ test/Tooling/clang-diff-json.cpp
@@ -3,9 +3,9 @@
 // RUN: | FileCheck %s
 
 // CHECK: "begin": 299,
-// CHECK: "type": "CXXRecordDecl",
 // CHECK: "type": "FieldDecl",
 // CHECK: "end": 319,
+// CHECK: "type": "CXXRecordDecl",
 class A {
   int x;
 };
Index: test/Tooling/clang-diff-ast.cpp
===
--- test/Tooling/clang-diff-ast.cpp
+++ test/Tooling/clang-diff-ast.cpp
@@ -12,7 +12,8 @@
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: CallExpr(
-  // CHECK: DeclRefExpr: f(
+  // CHECK-NOT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr: f(
   f();
   // CHECK: BinaryOperator: =(
   i = i;
@@ -37,6 +38,7 @@
 if (i == 0)
   // CHECK: StringLiteral: foo(
   return "foo";
+// CHECK-NOT: ImplicitCastExpr
 return 0;
   }
 
@@ -48,3 +50,22 @@
 int x = m;
   }
 };
+
+#define M (void)1
+#define MA(a, b) (void)a, b
+// CHECK: FunctionDecl
+// CHECK-NEXT: CompoundStmt
+void macros() {
+  M;
+  MA(1, 2);
+}
+// CHECK-NEXT: NamespaceDecl
+
+#ifndef GUARD
+#define GUARD
+namespace world {
+// nodes from other files are excluded
+// CHECK-NOT {{.}}
+#include "clang-diff-ast.cpp"
+}
+#endif
Index: lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- lib/Tooling/ASTDiff/ASTDiff.cpp
+++ lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -68,7 +68,8 @@
   // Compute ChangeKind for each node based on similarity.
   void computeChangeKinds(Mapping );
 
-  NodeId getMapped(const std::unique_ptr , NodeId Id) const {
+  NodeId getMapped(const std::unique_ptr ,
+   NodeId Id) const {
 if (&*Tree == )
   return TheMapping.getDst(Id);
 assert(&*Tree ==  && "Invalid tree.");
@@ -157,12 +158,23 @@
   void setLeftMostDescendants();
 };
 
+static bool isSpecializedNodeExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isSpecializedNodeExcluded(const Stmt *S) { return false; }
+
 template 
 static bool isNodeExcluded(const SourceManager , T *N) {
   if (!N)
 return true;
   SourceLocation SLoc = N->getLocStart();
-  return SLoc.isValid() && SrcMgr.isInSystemHeader(SLoc);
+  if (SLoc.isValid()) {
+// Ignore everything from other files.
+if (!SrcMgr.isInMainFile(SLoc))
+  return true;
+// Ignore macros.
+if (SLoc != SrcMgr.getSpellingLoc(SLoc))
+  return true;
+  }
+  return isSpecializedNodeExcluded(N);
 }
 
 namespace {
@@ -179,6 +191,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 ++Count;
@@ -241,6 +255,8 @@
 return true;
   }
   bool TraverseStmt(Stmt *S) {
+if (S)
+  S = S->IgnoreImplicit();
 if (isNodeExcluded(Tree.AST.getSourceManager(), S))
   return true;
 auto SavedState = PreTraverse(S);
@@ -900,7 +916,8 @@
   return TreeImpl->findPositionInParent(Id);
 }
 
-std::pair SyntaxTree::getSourceRangeOffsets(const Node ) const {
+std::pair
+SyntaxTree::getSourceRangeOffsets(const Node ) const {
   const SourceManager  = TreeImpl->AST.getSourceManager();
   SourceRange Range = N.ASTNode.getSourceRange();
   SourceLocation BeginLoc = Range.getBegin();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-09 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:177
+static bool isDeclExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isStmtExcluded(const Stmt *S) { return false; }
+

You should just use one call  `isNodeExcluded` that will redirect to 
node-specific overload, and avoid the multiple clauses in conditions, i.e..:

```
static bool isSpecializedNodeExcluded(const Decl *D) { }
static bool isSpecializedNodeExcluded(const Stmt *S) {}

template 
static bool isNodeExcluded(const SourceManager , T *N) {
    current code ...
   return isSpecializedNodeExcluded(N);
}
```

Then you can use `if (isNodeExcluded(Tree.AST.getSourceManager(), D))` 
everywhere.


https://reviews.llvm.org/D36184



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

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-02 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes added a comment.

In https://reviews.llvm.org/D36184#828866, @klimek wrote:

> Why? Also, missing tests.


implicit nodes are noisy / they generally don't add information; I guess one 
could also keep them.

I excluded nodes outside of the main file are because the visualisation only 
works with single files for now.
That will change, same as with macros.


https://reviews.llvm.org/D36184



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

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-02 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes updated this revision to Diff 109423.
johannes added a comment.

tests


https://reviews.llvm.org/D36184

Files:
  lib/Tooling/ASTDiff/ASTDiff.cpp
  test/Tooling/clang-diff-ast.cpp

Index: test/Tooling/clang-diff-ast.cpp
===
--- test/Tooling/clang-diff-ast.cpp
+++ test/Tooling/clang-diff-ast.cpp
@@ -12,7 +12,8 @@
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: CallExpr(
-  // CHECK: DeclRefExpr: f(
+  // CHECK-NOT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr: f(
   f();
   // CHECK: BinaryOperator: =(
   i = i;
@@ -37,6 +38,7 @@
 if (i == 0)
   // CHECK: StringLiteral: foo(
   return "foo";
+// CHECK-NOT: ImplicitCastExpr
 return 0;
   }
 
@@ -48,3 +50,22 @@
 int x = m;
   }
 };
+
+#define M (void)1
+#define MA(a, b) (void)a, b
+// CHECK: FunctionDecl
+// CHECK-NEXT: CompoundStmt
+void macros() {
+  M;
+  MA(1, 2);
+}
+// CHECK-NEXT: NamespaceDecl
+
+#ifndef GUARD
+#define GUARD
+namespace world {
+// nodes from other files are excluded
+// CHECK-NOT {{.}}
+#include "clang-diff-ast.cpp"
+}
+#endif
Index: lib/Tooling/ASTDiff/ASTDiff.cpp
===
--- lib/Tooling/ASTDiff/ASTDiff.cpp
+++ lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -162,24 +162,37 @@
   if (!N)
 return true;
   SourceLocation SLoc = N->getLocStart();
-  return SLoc.isValid() && SrcMgr.isInSystemHeader(SLoc);
+  if (!SLoc.isValid())
+return false;
+  // Ignore everything from other files.
+  if (!SrcMgr.isInMainFile(SLoc))
+return true;
+  // Ignore macros.
+  if (N->getLocStart() != SrcMgr.getSpellingLoc(N->getLocStart()))
+return true;
+  return false;
 }
 
+static bool isDeclExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isStmtExcluded(const Stmt *S) { return false; }
+
 namespace {
 /// Counts the number of nodes that will be compared.
 struct NodeCountVisitor : public RecursiveASTVisitor {
   int Count = 0;
   const SyntaxTree::Impl 
   NodeCountVisitor(const SyntaxTree::Impl ) : Tree(Tree) {}
   bool TraverseDecl(Decl *D) {
-if (isNodeExcluded(Tree.AST.getSourceManager(), D))
+if (isNodeExcluded(Tree.AST.getSourceManager(), D) || isDeclExcluded(D))
   return true;
 ++Count;
 RecursiveASTVisitor::TraverseDecl(D);
 return true;
   }
   bool TraverseStmt(Stmt *S) {
-if (isNodeExcluded(Tree.AST.getSourceManager(), S))
+if (S)
+  S = S->IgnoreImplicit();
+if (isNodeExcluded(Tree.AST.getSourceManager(), S) || isStmtExcluded(S))
   return true;
 ++Count;
 RecursiveASTVisitor::TraverseStmt(S);
@@ -233,15 +246,17 @@
   N.Height = std::max(N.Height, 1 + Tree.getNode(Child).Height);
   }
   bool TraverseDecl(Decl *D) {
-if (isNodeExcluded(Tree.AST.getSourceManager(), D))
+if (isNodeExcluded(Tree.AST.getSourceManager(), D) || isDeclExcluded(D))
   return true;
 auto SavedState = PreTraverse(D);
 RecursiveASTVisitor::TraverseDecl(D);
 PostTraverse(SavedState);
 return true;
   }
   bool TraverseStmt(Stmt *S) {
-if (isNodeExcluded(Tree.AST.getSourceManager(), S))
+if (S)
+  S = S->IgnoreImplicit();
+if (isNodeExcluded(Tree.AST.getSourceManager(), S) || isStmtExcluded(S))
   return true;
 auto SavedState = PreTraverse(S);
 RecursiveASTVisitor::TraverseStmt(S);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36184: [clang-diff] Filter AST nodes

2017-08-02 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek added a comment.

Just as a general note: adding cfe-commits after the fact is usually not a good 
idea, as we lose the history of the review in the email list (which is the 
source of truth).


https://reviews.llvm.org/D36184



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