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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
