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