[tooling] FixItHint Tooling refactoring

[etienneb]

Summary

This is the refactoring to lift some FixItHint into tooling.
used by: http://reviews.llvm.org/D19807

[etienneb]

If this is what you wanted Alex, I'll pursue writing unittests/docs and making a complete patch to land.

See some example on how to use it:
http://reviews.llvm.org/D19807

[alexfh]

In D19941#421735, @etienneb wrote:

If this is what you wanted Alex, I'll pursue writing unittests/docs and making a complete patch to land.

See some example on how to use it:
http://reviews.llvm.org/D19807

Yes, this looks reasonable.

include/clang/Tooling/Fixit.h

1

nit: Should be FixIt (note the capital I).

35

Do we want to expose getSourceRange as an API? If no, it should also be in the namespace internal.

55

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.

[etienneb]

add unittests

[etienneb]

more unittests

[alexfh]

include/clang/Tooling/Fixit.h

2

nit: s/FixIt Hint/FixItHint/, since this is a reference to the type.

lib/Tooling/CMakeLists.txt

10

Please rename the files as well (s/Fixit/FixIt/).

unittests/Tooling/CMakeLists.txt

15 ↗

(On Diff #56790)

s/FixitTest/FixItTest/

unittests/Tooling/FixitTest.cpp

34 ↗

(On Diff #56790)

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.

53 ↗

(On Diff #56790)

Could you add a test where getText returns an empty string?

119 ↗

(On Diff #56790)

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?

[etienneb]

address alexfh comments

[etienneb]

nits

[alexfh]

LG. Thanks!

unittests/Tooling/FixItTest.cpp

37 ↗

(On Diff #56826)

Let's either put the tests in the clang::tooling::<anonymous> namespace (not common for unit tests in this specific directory, but nevertheless reasonable, imo) or add using declarations for tooling::fixit::getText and other tested functions to reduce the amount of boilerplate.

[etienneb]

fix nits

[etienneb]

I added the "using statements".
landing.

include/clang/Tooling/Fixit.h

35

Good question.
I moved it into the internal namespace.

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

55

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.

unittests/Tooling/FixitTest.cpp

53 ↗

(On Diff #56790)

see line 68, already there!

119 ↗

(On Diff #56790)

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>",

include/clang/Tooling/Fixit.h

+//===--- Fixit.h - Fixit Hint utilities -------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements functions to ease source rewriting from AST-nodes.
+//
+//  Example swapping A and B expressions:
+//
+//    Expr *A, *B;
+//    tooling::fixit::createReplacement(*A, *B);
+//    tooling::fixit::createReplacement(*B, *A);
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_FIXIT_H
+#define LLVM_CLANG_TOOLING_FIXIT_H
+
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context);
+} // end namespace internal
+
+/// \brief Returns the SourceRange of a SourceRange. This identity function is
+///        used by the following template abstractions.
+inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+
+/// \brief Returns the SourceRange of the token at Location \p Loc.
+inline SourceRange getSourceRange(const SourceLocation &Loc) {
+  return SourceRange(Loc);
+}
+
+/// \brief Returns the SourceRange of an given Node. \p Node is typically a
+///        'Stmt', 'Expr' or a 'Decl'.
+template <typename T> SourceRange getSourceRange(const T &Node) {
+  return Node.getSourceRange();
+}
+
+// \brief Returns a textual representation of \p Node.
+template <typename T>
+StringRef getText(const T &Node, const ASTContext &Context) {
+  return getText(getSourceRange(Node), Context);
+}
+
+// \brief Returns a FixItHint to remove \p Node.
+template <typename T> FixItHint createRemoval(const T &Node) {
+  return FixItHint::CreateRemoval(getSourceRange(Node));
+}
+
+// \brief Returns a FixItHint to replace \p Destination by \p Source.
+template <typename D, typename S>
+FixItHint createReplacement(const D &Destination, const S &Source,
+                            const ASTContext &Context) {
+  return FixItHint::CreateReplacement(getSourceRange(Destination),
+                                      getText(Source, Context));
+}
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_FIXINT_H

lib/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_subdirectory(Core)
 
 add_clang_library(clangTooling
   ArgumentsAdjusters.cpp
   CommonOptionsParser.cpp
   CompilationDatabase.cpp
   FileMatchTrie.cpp
+  Fixit.cpp
   JSONCompilationDatabase.cpp
   Refactoring.cpp
   RefactoringCallbacks.cpp
   Tooling.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangDriver
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangToolingCore
   )

lib/Tooling/Fixit.cpp

+//===--- RefactoringCallbacks.cpp - Structural query framework ------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//
+//===----------------------------------------------------------------------===//
+#include "clang/Tooling/Fixit.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                              Context.getSourceManager(),
+                              Context.getLangOpts());
+}
+} // end namespace internal
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang