[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 12 04:42:25 PDT 2017

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.


EXPECT_TRUE(...); // function f definition
EXPECT_TRUE(...); // function body of function f



More information about the cfe-commits mailing list