[clang-diff] Filter AST nodes

[johannes]

Summary

Ignore macros and implicit AST nodes, as well as anything outside of the
main source file.

[johannes]

parent changed

[klimek]

Why? Also, missing tests.

[klimek]

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).

[johannes]

tests

[johannes]

In 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.

[arphaman]

lib/Tooling/ASTDiff/ASTDiff.cpp

181

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 <class T>
static bool isNodeExcluded(const SourceManager &SrcMgr, T *N) {
   .... current code ...
   return isSpecializedNodeExcluded(N);
}

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

[johannes]

refactor isNodeExcluded

[arphaman]

test/Tooling/clang-diff-ast.cpp

68

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

[johannes]

test/Tooling/clang-diff-ast.cpp

68

I want to assert that there is no output here, because other files are excluded, there may be a better way..

[johannes]

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

[arphaman]

LGTM.

test/Tooling/clang-diff-ast.cpp

68

Ok, fair enough.

lib/Tooling/ASTDiff/ASTDiff.cpp

Impl(SyntaxTree::Impl &T1, SyntaxTree::Impl &T2,
        const ComparisonOptions &Options);
 
   /// Matches nodes one-by-one based on their similarity.
   void computeMapping();
 
   // Compute ChangeKind for each node based on similarity.
   void computeChangeKinds(Mapping &M);
 
-  NodeId getMapped(const std::unique_ptr<SyntaxTree::Impl> &Tree, NodeId Id) const {
+  NodeId getMapped(const std::unique_ptr<SyntaxTree::Impl> &Tree,
+                   NodeId Id) const {
     if (&*Tree == &T1)
       return TheMapping.getDst(Id);
     assert(&*Tree == &T2 && "Invalid tree.");
     return TheMapping.getSrc(Id);
   }
 
 private:
   // Returns true if the two subtrees are identical.

 private:
   /// Nodes in preorder.
   std::vector<Node> Nodes;
 
   void initTree();
   void setLeftMostDescendants();
 };
 
+static bool isSpecializedNodeExcluded(const Decl *D) { return D->isImplicit(); }
+static bool isSpecializedNodeExcluded(const Stmt *S) { return false; }
+
 template <class T>
 static bool isNodeExcluded(const SourceManager &SrcMgr, 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 {
 /// Counts the number of nodes that will be compared.
 struct NodeCountVisitor : public RecursiveASTVisitor<NodeCountVisitor> {
   int Count = 0;
   const SyntaxTree::Impl &Tree;
   NodeCountVisitor(const SyntaxTree::Impl &Tree) : Tree(Tree) {}
   bool TraverseDecl(Decl *D) {
     if (isNodeExcluded(Tree.AST.getSourceManager(), D))
       return true;
     ++Count;
     RecursiveASTVisitor<NodeCountVisitor>::TraverseDecl(D);
     return true;
   }
   bool TraverseStmt(Stmt *S) {
+    if (S)
+      S = S->IgnoreImplicit();
     if (isNodeExcluded(Tree.AST.getSourceManager(), S))
       return true;
     ++Count;
     RecursiveASTVisitor<NodeCountVisitor>::TraverseStmt(S);
     return true;
   }
   bool TraverseType(QualType T) { return true; }
 };
bool TraverseDecl(Decl *D) {
     if (isNodeExcluded(Tree.AST.getSourceManager(), D))
       return true;
     auto SavedState = PreTraverse(D);
     RecursiveASTVisitor<PreorderVisitor>::TraverseDecl(D);
     PostTraverse(SavedState);
     return true;
   }
   bool TraverseStmt(Stmt *S) {
+    if (S)
+      S = S->IgnoreImplicit();
     if (isNodeExcluded(Tree.AST.getSourceManager(), S))
       return true;
     auto SavedState = PreTraverse(S);
     RecursiveASTVisitor<PreorderVisitor>::TraverseStmt(S);
     PostTraverse(SavedState);
     return true;
   }
   bool TraverseType(QualType T) { return true; }
SyntaxTree::PreorderIterator SyntaxTree::begin() const {
   return TreeImpl->begin();
 }
 SyntaxTree::PreorderIterator SyntaxTree::end() const { return TreeImpl->end(); }
 
 int SyntaxTree::findPositionInParent(NodeId Id) const {
   return TreeImpl->findPositionInParent(Id);
 }
 
-std::pair<unsigned, unsigned> SyntaxTree::getSourceRangeOffsets(const Node &N) const {
+std::pair<unsigned, unsigned>
+SyntaxTree::getSourceRangeOffsets(const Node &N) const {
   const SourceManager &SrcMgr = TreeImpl->AST.getSourceManager();
   SourceRange Range = N.ASTNode.getSourceRange();
   SourceLocation BeginLoc = Range.getBegin();
   SourceLocation EndLoc = Lexer::getLocForEndOfToken(
       Range.getEnd(), /*Offset=*/0, SrcMgr, TreeImpl->AST.getLangOpts());
   if (auto *ThisExpr = N.ASTNode.get<CXXThisExpr>()) {
     if (ThisExpr->isImplicit())
       EndLoc = BeginLoc;

test/Tooling/clang-diff-ast.cpp

 // RUN: clang-diff -ast-dump %s -- -std=c++11 | FileCheck %s
 
 
 // CHECK: {{^}}TranslationUnitDecl(0)
 // CHECK: {{^}} NamespaceDecl: test;(
 namespace test {
 
 // CHECK: {{^}}  FunctionDecl: f(
 // CHECK: CompoundStmt(
 void f() {
   // CHECK: VarDecl: i(int)(
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: CallExpr(
-  // CHECK: DeclRefExpr: f(
+  // CHECK-NOT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr: f(
   f();
   // CHECK: BinaryOperator: =(
   i = i;
 }
 
 } // end namespace test
 
 // CHECK: TypedefDecl: nat;unsigned int;(
 typedef unsigned nat;
 // CHECK: TypeAliasDecl: real;double;(
 using real = double;
 
 class Base {
 };
 
 // CHECK: CXXRecordDecl: X;class X;(
 class X : Base {
   int m;
   // CHECK: CXXMethodDecl: foo(const char *(int))(
   // CHECK: ParmVarDecl: i(int)(
   const char *foo(int i) {
     if (i == 0)
       // CHECK: StringLiteral: foo(
       return "foo";
+    // CHECK-NOT: ImplicitCastExpr
     return 0;
   }
 
   // CHECK: AccessSpecDecl: public(
 public:
   // CHECK: CXXConstructorDecl: X(void (char, int))(
   X(char, int) : Base(), m(0) {
     // CHECK: MemberExpr(
     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

test/Tooling/clang-diff-json.cpp

 // RUN: clang-diff -ast-dump-json %s -- \
 // RUN: | %python -c 'import json, sys; json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN: | FileCheck %s
 
 // CHECK: "begin": 299,
-// CHECK: "type": "CXXRecordDecl",
 // CHECK: "type": "FieldDecl",
 // CHECK: "end": 319,
+// CHECK: "type": "CXXRecordDecl",
 class A {
   int x;
 };
 
 // CHECK: "children": [
 // CHECK-NEXT: {
 // CHECK-NEXT: "begin":
 // CHECK-NEXT: "children": []