[PATCH] D34748: [clang-diff] improve mapping accuracy

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In general this patch is too big, and combines a lot of changes that could be separated using multiple patches. I've suggested one pre-commit and a new patch so far, but more changes might be required once I see the updated patch.



================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:48
+  bool isLeaf() const { return Children.empty(); }
+  llvm::Optional<const std::string> getQualifiedIdentifier() const;
+  llvm::Optional<const StringRef> getIdentifier() const;
----------------
Redundant `const` in the return type.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:49
+  llvm::Optional<const std::string> getQualifiedIdentifier() const;
+  llvm::Optional<const StringRef> getIdentifier() const;
+};
----------------
Redundant `const` in the return type.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:58
 
-  // Returns a list of matches.
-  std::vector<Match> getMatches();
-  /// Returns an edit script.
-  std::vector<Change> getChanges();
+  SyntaxTree &SrcTree, &DstTree;
 
----------------
Please Move these fields to the end of the class. Can they be private too?


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:170
+  // Ignore everything from other files.
+  if (!SrcMgr.isInMainFile(SLoc))
+    return true;
----------------
How will you deal with changes in headers if you limit analysis to main files only? 


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:694
+
+const StringRef Node::getTypeLabel() const { return getType().asStringRef(); }
+
----------------
Redundant `const` in return type.


================
Comment at: tools/clang-diff/ClangDiff.cpp:28
 static cl::opt<bool>
-    DumpAST("ast-dump",
+    ASTDump("ast-dump",
             cl::desc("Print the internal representation of the AST as JSON."),
----------------
The rename of `DumpAST` to `ASTDump` should be a separate change. Please pre-commit this rename and remove it from the updated patch.


================
Comment at: tools/clang-diff/ClangDiff.cpp:45
 
+static cl::opt<bool> NoCompilationDatabase(
+    "no-compilation-database",
----------------
Can you avoid moving the `NoCompilationDatabase`? This will make the patch clearer.


================
Comment at: tools/clang-diff/ClangDiff.cpp:65
+namespace {
+class ArgumentsAdjustingCompilations : public CompilationDatabase {
+public:
----------------
This is a copy `ArgumentsAdjustingCompilations` from lib/Tooling. You should create a patch that moves existing `ArgumentsAdjustingCompilations` from the implementation file to some header so that it will be accessible by the tool and make this patch depend on the new patch.


================
Comment at: tools/clang-diff/ClangDiff.cpp:224
+  case diff::None:
+  case diff::Delete:
+    break;
----------------
You might want to assert here. We should never have `Delete` in the destination AST, right?


https://reviews.llvm.org/D34748