This is an archive of the discontinued LLVM Phabricator instance.

[clang-diff] Initial implementation of patching
Needs ReviewPublic

Authored by johannes on Aug 22 2017, 2:17 AM.

Details

Reviewers
arphaman
Summary

This commit adds a function diff::patch that takes three syntax trees
as inputs: a source, a destination and a target. It tries to apply the
differences between source and destination to target. Currently,
comments and preprocessor statements are ignored.

Event Timeline

johannes created this revision.Aug 22 2017, 2:17 AM
arphaman edited edge metadata.Aug 25 2017, 6:12 AM

I don't think AST manipulation is the right way to do patching. You've already hit the limitations here in this patch where you can't really remove a statement unless its parent is a CompoundStmt.

I think the right way to do AST patching is to take a 3rd AST(lets call it a target AST), find the set of matching nodes that correspond to all nodes that have to be patched and then perform patching by rewriting the source for the target AST.
Let's say you want to start with remove. If you take the following files:

$ cat src.cpp
void printf(const char *, ...);
void foo(int x) {
  printf("%d", x, x);
}
$ cat dst.cpp
void printf(const char *, ...);
void foo(int x) {
  printf("%d", x); // the second 'x' is removed.
}
$ cat target.cpp
void printf(const char *, ...);
void foo(int x) {
  printf("different string %d", x, x);
}

You'll find that the difference between src.cpp and dst.cpp is Delete DeclRefExpr: x(10). Then you can take target.cpp, find the matching DeclRefExpr node, and create a source replacement (see tooling's Replacement) that removes ", x" from target.cpp.

johannes updated this revision to Diff 112845.Aug 27 2017, 4:02 PM
johannes retitled this revision from Add include/clang/Tooling/ASTDiff/ASTPatch.h to [clang-diff] Initial implementation of patching.
johannes edited the summary of this revision. (Show Details)

use rewriter to patch a third AST

arphaman added inline comments.Aug 28 2017, 3:50 AM
lib/Tooling/ASTDiff/ASTDiff.cpp
658

You can drop the auto *Arg since Arg is unused.

1305

Please move the patcher into a new file.

johannes updated this revision to Diff 112926.Aug 28 2017, 11:41 AM
johannes edited the summary of this revision. (Show Details)

split to ASTDiff/ASTPatch

arphaman added inline comments.Aug 29 2017, 5:32 AM
include/clang/Tooling/ASTDiff/ASTPatch.h
1 ↗(On Diff #112926)

Please update the file name in the comment

lib/Tooling/ASTDiff/ASTDiff.cpp
659

You might as well return early here and avoid else entirely

arphaman added inline comments.Aug 29 2017, 5:35 AM
include/clang/Tooling/ASTDiff/ASTDiff.h
76

Why do you need to create an empty tree? What about using llvm::Optional instead?

85

It's fine to have = default in the header

johannes updated this revision to Diff 113078.Aug 29 2017, 6:22 AM

fixes

include/clang/Tooling/ASTDiff/ASTDiff.h
76

ok, i use optional now instead in the unittest

johannes edited the summary of this revision. (Show Details)Nov 5 2017, 11:54 AM