alexfh added inline comments.
================
Comment at: include/clang/Tooling/Fixit.h:1
@@ +1,2 @@
+//===--- FixIt.h - FixIt Hint utilities -------------------------*- C++
-*-===//
+//
----------------
nit: s/FixIt Hint/FixItHint/, since this is a reference to the type.
================
Comment at: lib/Tooling/CMakeLists.txt:10
@@ -9,2 +9,3 @@
FileMatchTrie.cpp
+ Fixit.cpp
JSONCompilationDatabase.cpp
----------------
Please rename the files as well (s/Fixit/FixIt/).
================
Comment at: unittests/Tooling/CMakeLists.txt:15
@@ -14,2 +14,3 @@
CompilationDatabaseTest.cpp
+ FixitTest.cpp
LookupTest.cpp
----------------
s/FixitTest/FixItTest/
================
Comment at: unittests/Tooling/FixitTest.cpp:34
@@ +33,3 @@
+
+ Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+ EXPECT_EQ("foo(x, y)", tooling::fixit::getText(*CE, *Context));
----------------
Automatic captures always seem suspicious to me. What exactly does this lambda
need to capture? If just `this`, I'd rather make the capture list explicit.
================
Comment at: unittests/Tooling/FixitTest.cpp:53
@@ +52,3 @@
+
+TEST(FixitTest, getTextWithMacro) {
+ CallsVisitor Visitor;
----------------
Could you add a test where getText returns an empty string?
================
Comment at: unittests/Tooling/FixitTest.cpp:119
@@ +118,3 @@
+ LocationToString(Hint0.RemoveRange.getBegin(), Context));
+ EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+ LocationToString(Hint0.RemoveRange.getEnd(), Context));
----------------
One character range is boring. Can you make the parameter longer and also
verify whether the range is a token range or a character range?
http://reviews.llvm.org/D19941
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
