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

Wed Jun 26 08:13:17 PDT 2019

kadircet added inline comments.

================
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) {
----------------
SureYeaah wrote:
> kadircet wrote:
> > 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?
> Right now, we check whether we will need braces and if so, we don't provide an extraction option. Although we don't insert the braces, we still need to check if the extraction would require braces.
> 
> The "body" of an if, for, while etc. will either be a CompoundStmt in which case there are already braces or not in which case there are no braces. So if while going up the AST parents, we encounter a CompoundStmt, we stop there. Otherwise, if we encounter an if/for/while etc. Stmt, we we have 2 cases - Either we are in the body (e.g. do <body> while(...); ) or the condition part (e.g. if(...) or for(...)). So we check which of these two cases it is and accordingly decide if we need braces i.e. the body case.
> 
> So we don't need to check whether it's the only statement. Does that make sense?
ah I see, I was looking at this code in isolation so didn't notice the check for hitting a compoundstmt first. it makes sense now.

================
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) {
----------------
SureYeaah wrote:
> kadircet wrote:
> > 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` ?
> The reason why it's specific for ForStmt (at least for now) is because only in ForStmt we can have declarations as well as other expressions in the for(...) part. For all other cases, we don't have such a scenario. WDYT?
it is also possible to have a similar issue with

```
int a = 5, b = [[a + 2]];
```

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:122
+// is a CompoundStmt, we can extract inside this CompoundStmt just above the
+// current Stmt. We ALWAYS insert above a Stmt whose parent is a CompoundStmt
+//
----------------
s/above/just before

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:158
+    }
+    CurN = CurN->Parent;
+  }
----------------
maybe move this into `if (!ParStmt)` case ?

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:186
+  std::string ExtractedVarDecl =
+      std::string("auto ") + VarName + " = " + ExpCode.str() + "; ";
+  return tooling::Replacement(M, StmRng->getBegin(), 0, ExtractedVarDecl);
----------------
maybe add a todo for replacing auto with explicit type?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773