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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
