[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API.



================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:74
+/// \see CodeRangeASTSelection
+class CodeRangeSelectionRequirement : public ASTSelectionRequirement {
+public:
----------------
Could we find a better name for this? It's not clear what the difference between this and `SourceRangeSelectionRequirement` is from the names.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringRuleContext.h:84
+  // FIXME: Remove when memoized.
+  std::unique_ptr<SelectedASTNode> ASTSelection;
 };
----------------
Maybe `ASTNodeSelection` for clarity?


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:340
+  bool IsPrevCompound = false;
+  for (const auto &Parent : llvm::reverse(Parents)) {
+    const DynTypedNode &Node = Parent.get().Node;
----------------
A short explanation of the for-loop would be appreciated :)


================
Comment at: lib/Tooling/Refactoring/Extract.cpp:167
+      OS << "return ";
+    OS << ExtractedCodeRewriter.getRewrittenText(ExtractedRange);
+    // FIXME: Compute the correct semicolon policy.
----------------
An alternative way to get the source code is:
``` 
Lexer::getSourceText(
      CharSourceRange::getTokenRange(SM.getSpellingLoc(Start), SM.getSpellingLoc(End)),
      SM, Result.Context->getLangOpts());
```


================
Comment at: tools/clang-refactor/TestSupport.cpp:106
+    auto Buf = llvm::MemoryBuffer::getMemBuffer(Result->c_str());
+    for (llvm::line_iterator It(*Buf, /*SkipBlanks=*/false); !It.is_at_end();
+         ++It) {
----------------
Can we filter the `CHECK`s with `sed` in the test? Similar to https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/change-namespace/simple-move.cpp#L1


Repository:
  rL LLVM

https://reviews.llvm.org/D38982