[PATCH] D19941: [tooling] FixItHint Tooling refactoring

[Alexander Kornienko via cfe-commits]

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