hokein added inline comments.
================
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:74
+/// An AST selection value that corresponds to a selection of a set of
+/// statements that belong to one body of code (like one function).
+///
----------------
I see all your tests are for function body, does it support other body, i.e.
"global namespace", "compound statement"?
if yes, how about adding more test cases to cover it.
```
// variable in global namespace
int a; // select int.
```
```
void f() {
{
int a; // select a.
}
}
```
================
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:95
+/// \c SelectedASTNode tree and should not outlive it.
+struct CodeRangeASTSelection {
+ CodeRangeASTSelection(CodeRangeASTSelection &&) = default;
----------------
nit: instead of a structure, this feels more like a `class`.
================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:247
+namespace {
+
+struct SelectedNodeWithParents {
----------------
nit: remove the empty line.
================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:685
+)";
+ // Empty range is an invalid code range.
+ findSelectedASTNodesWithRange(
----------------
I think "No selection range" is more precise -- you are passing `None`
parameter to the function.
For `empty range`, it should be something like `FileRange{{2, 2}, {2, 2}}`, we
can also add a test for it.
================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:688
+ Source, {2, 2}, None,
+ [](SourceRange SelectionRange, Optional<SelectedASTNode> Node) {
+ EXPECT_TRUE(Node);
----------------
I'm a bit confused here, if the selection range is none/empty, shouldn't
`SelectedASTNode` be empty?
If this behavior is intended, I'd suggest documenting it in
`findSelectedASTNodesWithRange`.
================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:718
+ isa<TranslationUnitDecl>(Parents[0].get().Node.get<Decl>()));
+ EXPECT_TRUE(isa<FunctionDecl>(Parents[1].get().Node.get<Decl>()));
+ EXPECT_TRUE(isa<CompoundStmt>(Parents[2].get().Node.get<Stmt>()));
----------------
nit: would be clearer to add a comment indicating the corresponding code, the
same below.
like
```
EXPECT_TRUE(...); // function f definition
EXPECT_TRUE(...); // function body of function f
```
Repository:
rL LLVM
https://reviews.llvm.org/D38835
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
