[PATCH] D65526: [Clangd] First version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:46
+//    creating a NewFunction.
+// Design Doc at <TODO: Share design doc>
+//===----------------------------------------------------------------------===//
----------------
please fix or remove comment


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:88
+
+// The ExtractionZone class forms a view of the code wrt to Zone.
+// A RootStmt is a statement that's fully selected including all it's children
----------------
nit: "wrt to" -> wrt


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:104
+  SourceRange EnclosingFuncRange;
+  const SourceManager &SM;
+
----------------
remove from struct and pass as a parameter?
This isn't really data


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:112
+  }
+  const Node *getLastRootStmt() const {
+    return Parent ? Parent->Children.back() : nullptr;
----------------
document why this is important, or just inline it?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:113
+  const Node *getLastRootStmt() const {
+    return Parent ? Parent->Children.back() : nullptr;
+  }
----------------
don't bother handling the null parent case. When can that happen? We should avoid creating an ExtractionZone struct entirely then.

Also none of the callsites bother to check for null...


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:132
+}
+// Finds the function in which the zone lies.
+const Node *computeEnclosingFunction(const Node *CommonAnc) {
----------------
nit: consistently one blank line between functions (in this file you alternate between 1/0)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:157
+  SourceRange SR;
+  for (const Node *Child : Parent->Children) {
+    auto ChildFileRange =
----------------
why are you doing this with a loop, isn't it just {first->begin,last->end}?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+  for (const Node *Child : Parent->Children) {
----------------
kadircet wrote:
> `hasOnlyRootStmtsAsChildren` ?
nit: I think this would be clearer as
`canBeRootStmt(const Node*)` and write the callsite as `llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc,
+                                                  const SourceManager &SM,
----------------
kadircet wrote:
> why is this function returning a shared_ptr ?
avoid shared_ptr unless there's a strong reason. `Optional<ExtractionZone>` seems fine here?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:257
+    unsigned OrderPriority; // Lower value parameters are preferred first.
+    std::string render(bool WithTypeAndQualifiers) const;
+    bool operator<(const Parameter &Other) const {
----------------
there's no need for the WithTypeAndQualifiers=false version, that's just "fn.name"


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+  tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+  NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+              tooling::ExtractionSemicolonPolicy SemicolonPolicy,
----------------
this is just initializing public fields, drop the constructor?
(The callsite is clearer here if you initialize them by name)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:280
+  // Add a new parameter for the function.
+  void addParam(llvm::StringRef Name, QualType TypeInfo, bool IsConst,
+                bool IsReference, unsigned OrderPriority);
----------------
this is just push_back, inline into caller


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:285
+  const SourceManager &SM;
+  std::string renderParameters(bool WithTypeAndQualifiers) const;
+  // Generate the function body.
----------------
again, avoid sharing code between the complicated declaration case, and the trivial callsite case. It makes changes harder and obscures the hard case.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+  return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};
----------------
use `printType` from AST.h?

(You'll want to drop qualifiers/refs before calling that, but it's not at all obvious from the function name here that they're dropped, so that should be at the callsite anyway)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+    const Decl *TheDecl = nullptr;
+    ZoneRelative DeclaredIn;
+    // Stores the numbering of this declaration(i for the i-th Decl)
----------------
missing initializers on this and DeclIndex


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:347
+    ZoneRelative DeclaredIn;
+    // Stores the numbering of this declaration(i for the i-th Decl)
+    unsigned DeclIndex;
----------------
kadircet wrote:
> what does `numbering` mean ?
Giving a definition of "index" isn't useful here :-)
Rather prefer to say what it's the index of: is it the index of first reference, relative to the other decls?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:352
+    // FIXME: Capture mutation information
+    DeclInformation(){};
+    DeclInformation(const Decl *TheDecl, ZoneRelative DeclaredIn,
----------------
what is this constructor for?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:375
+                                        const ExtractionZone &ExtZone) {
+  // Adding a new Decl (The declarationIndex will be the size of the
+  // DeclInfoMap).
----------------
Again, the comment just echoes the code. There is some subtlety here, either explain it ("the index is the size of the map so far, so it increases for each decl found") or ignore it - this isn't too hard to work out.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:410
+    }
+    bool VisitDecl(Decl *D) { // NOLINT
+      // Create new DeclInfo
----------------
please don't add these NOLINT comments everywhere. It's too noisy and we don't do it elsewhere (agree we should silence clang-tidy somehow)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:430
+    bool VisitBreakStmt(BreakStmt *Break) { // NOLINT
+      if (isStmtInExtractionZone(Break, ExtZone))
+        Info.HasBreakOrContinue = true;
----------------
computing and checking bounds for every node in the same function seems unneccesarily inefficient.

Why not override TraverseStmt to set a flag to true when the stmt is a RootStmt, call Base::TraverseStmt then set it to false again after?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:455
+    const ValueDecl *VD = dyn_cast_or_null<ValueDecl>(DeclInfo.TheDecl);
+    bool WillBeParameter = (DeclInfo.DeclaredIn == ZoneRelative::Before &&
+                            DeclInfo.IsReferencedInZone) ||
----------------
seems you could write this more directly - e.g. it's natural to check for hoisting here.

```
if (DeclaredIn == Inside && ReferencedInPostZone)
  return false; // needs to be hoisted, not supported
if (!ReferencedInZone)
  continue; // no need to pass as parameter, not referenced
if (DeclaredIn == Inside || DeclaredIn == OutsideFunc)
  continue; // no need to pass as parameter, still accessible
```



================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:611
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
----------------
wait, what?
expressions that *aren't statements* shouldn't be extracted.
But what's wrong with this one?
(Many useful statements are expressions, such as function calls)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:615
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+  // FIXME: Add tests for macros after selectionTree works properly for macros.
+  // FIXME: This should be extractable
----------------
please clarify this comment (specifically what needs to be fixed?) ideally add the broken test case with a FIXME to fix it


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:621
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[{}]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
----------------
add a more realistic example? I can easily imagine banning this case in the future (extraction of an empty block)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:641
+  std::string ParameterCheckInput = R"cpp(
+struct FOO {
+  int x;
----------------
please make the tests use reasonably conventional style - FOO -> Foo, one statement per line, etc


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526