[PATCH] D63773: [clangd] dummy variable extraction on a function scope

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:54
+
+// RAV subclass to find all DeclRefs in a given Stmt
+class FindDeclRefsVisitor
----------------
I believe this class is rather used to check if any decl referenced in an expression was declared in a statement, rather then finding those declrefs.

So what about turning it inside out :D ? Something like:

```
bool isDeclaredIn(const DeclStmt *DS, const Expr *E, SourceManager &SM) {
  // Collects all DelcRefs in an AST node.
  class FindDeclRefsVisitor .... {
  public: 
    bool VisitDeclRefExpr(... *DRE) { DeclRefs.push_back(DRE); return true; }
    std::vector<DelcRefExpr*> DeclRefs;
  };
  FindDeclRefsVisitor X;
  X.TraverseStmt ...
  for(auto *DRE : X.DeclRefs) {
     ...
   }
   return ...;
}
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:80
+
+// Here T is a Stmt subclass and getBody is a member function that returns its
+// "body" Stmt.
----------------
looks really nice but we already type all of the member fields and types in the code explicitly anyway, maybe simplify a bit with:
```
static bool needBraces(....) {
  auto IsNodeInside = [..](SourceRange Body) {
     return M.isPointWithin(...);
  };

 if(ForStmt *Stmt = llvm::dyn_cast_or_null<ForStmt>(Stm))
    return IsNodeInside(Stmt->getBody());
  ...
  ...
}
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:95
+// Returns true if we will need braces after extraction
+static bool needBraces(const SelectionTree::Node *N, const Stmt *Stm,
+                       const SourceManager &M) {
----------------
I suppose currently this check is not doing what it claims to do(not checking for whether it is the only statement and/or if there are enclosing braces).

Maybe leave it out of this patch completely?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:108
+// check whether to allow extraction from for(...)
+static bool checkFor(const ForStmt *F, const Expr *Exp,
+                     const SourceManager &M) {
----------------
I believe it would be better to have a more generic check that will ensure all of the delcs referenced in `Exp` are still visible in the context which we'll insert the new statement. WDYT?

Were there any reasons to make it specific for `ForStmt` ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:119
+// Return the Stmt before which we need to insert the extraction.
+static const clang::Stmt *getStmt(const SelectionTree::Node *N,
+                                  const SourceManager &M) {
----------------
could you add a few comments explaining the logic?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137
+                                                  N->ASTNode.get<Expr>(), M)) {
+      // Check whether the expression references any variable in the for
+      // initializer and if so, we can't extract
----------------
what about a case like:
```
for(int i=0;;) {
  int z = [[f(i)*10]] + 5;
}
```
I believe it should be OK to extract in this case right ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:177
+  // insert new variable declaration
+  if (auto Err = Result.add(tooling::Replacement(
+          Ctx.getSourceManager(), StmRng->getBegin(), 0, ExtractedVarDecl)))
----------------
maybe extract these into a function that will take `VarName`, `Stm` and `Exp` as parameters and will return an `Expected<Replacement>`. So that you can easily change "dummy" part later on if we get another way to invoke this functionality.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:181
+  // replace expression with variable name
+  if (auto Err = Result.add(tooling::Replacement(
+          Ctx.getSourceManager(), ExpRng->getBegin(), ExpCode.size(), "dummy")))
----------------
same as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773