Page MenuHomePhabricator

[clang-diff] Filter AST nodes
ClosedPublic

Authored by johannes on Aug 1 2017, 1:55 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

johannes created this revision.Aug 1 2017, 1:55 PM
johannes updated this revision to Diff 109307.Aug 2 2017, 2:57 AM

parent changed

klimek added a comment.Aug 2 2017, 3:46 AM

Why? Also, missing tests.

klimek added a comment.Aug 2 2017, 6:08 AM

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

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 added inline comments.Aug 9 2017, 4:43 AM
lib/Tooling/ASTDiff/ASTDiff.cpp
177 ↗(On Diff #109423)

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 updated this revision to Diff 110555.Aug 10 2017, 4:19 AM

refactor isNodeExcluded

arphaman added inline comments.Aug 10 2017, 10:34 AM
test/Tooling/clang-diff-ast.cpp
68 ↗(On Diff #110555)
  1. Missing ':'
  2. What exactly does this regex accomplish? Right now it will match any character which doesn't look correct
johannes added inline comments.Aug 10 2017, 10:36 AM
test/Tooling/clang-diff-ast.cpp
68 ↗(On Diff #110555)

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

johannes updated this revision to Diff 110687.Aug 11 2017, 4:37 AM

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

arphaman accepted this revision.Aug 11 2017, 8:29 AM

LGTM.

test/Tooling/clang-diff-ast.cpp
68 ↗(On Diff #110555)

Ok, fair enough.

This revision is now accepted and ready to land.Aug 11 2017, 8:29 AM
This revision was automatically updated to reflect the committed changes.