[PATCH] D19941: [tooling] FixItHint Tooling refactoring

[Etienne Bergeron via cfe-commits]

etienneb added a comment.

I added the "using statements".
landing.


================
Comment at: include/clang/Tooling/Fixit.h:35
@@ +34,3 @@
+///        used by the following template abstractions.
+inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+
----------------
alexfh wrote:
> Do we want to expose `getSourceRange` as an API? If no, it should also be in the `namespace internal`.
Good question.
I moved it into the internal namespace.

If it's become needed, we can re-expose it.

================
Comment at: include/clang/Tooling/Fixit.h:55
@@ +54,3 @@
+// \brief Returns a FixItHint to remove \p Node.
+template <typename T> FixItHint createRemoval(const T &Node) {
+  return FixItHint::CreateRemoval(getSourceRange(Node));
----------------
alexfh wrote:
> I wonder whether we should later extend this to remove comments attached to nodes like Stmt and FunctionDecl? It's out of this patches scope, but maybe add a FIXME to sketch the further development plans.
I like the idea. I didn't think about comments and other syntactical elements that may be related to a Node. 
But, I believe we will have a other set of functions for Node and comments.

An other concept that I believe it's missing is to provide helper to append expression (operators) and stay semantically correct with operator precedence. Some cases needed to add parentheses.



================
Comment at: unittests/Tooling/FixitTest.cpp:53
@@ +52,3 @@
+
+TEST(FixitTest, getTextWithMacro) {
+  CallsVisitor Visitor;
----------------
alexfh wrote:
> Could you add a test where getText returns an empty string?
see line 68, already there!

================
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));
----------------
alexfh wrote:
> 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?
In this case, it's not one character... it's one token.
As it's a "token range", the whole token is used.

But, I can use an expression too.
Not a bad idea: added but in 'createRemoval'

There is already a reand at line 138:
  EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:37>",
  EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:45>",


http://reviews.llvm.org/D19941