[PATCH] D34329: [GSoC] Clang AST diffing

[Johannes Altmanninger via Phabricator via cfe-commits]

johannes added a comment.

In https://reviews.llvm.org/D34329#784090, @arphaman wrote:

> Generally we shouldn't have untested code in trunk, so I think that we need to find a way to test this before committing. We can start off by testing the output of the diff tool. Since there will be a lot of changes in the future, you don't have to have everything covered now, so I think that even a couple of tests be enough.


Thanks for your feedback! I think I have adressed all issues, except for the tests.

For the tests to run properly, I tried to create a local compile_commands.json, because ClangTool refuses to build an AST when the command for a source not found in the compilation database. However, it seems like relative paths do not work for the "directory" property.
So maybe this can be added? Other options are: 1. patching the compilation database before running the test, so that it has the absolute path or 2. adding an option to my tool to not load a compilation database. WDYT?



================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:31
+
+static bool isRelevantNode(Decl *D) { return D != nullptr; }
+static bool isRelevantNode(Stmt *S) { return S != nullptr; }
----------------
arphaman wrote:
> I don't see the point in these functions. Are you going to add some more logic to them?
> 
> I'd suggest removing them and using early return in the traversal functions instead, e.g:
> 
> ```
>   bool TraverseDecl(Decl *D) {
>     if (!D)
>       return true;
>     ++Count;
>     return RecursiveASTVisitor<NodeCountVisitor>::TraverseDecl(D);
>   }
> ```
This is now done in `discardNode`, which also filters nodes from system headers.


https://reviews.llvm.org/D34329