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

Wed Jun 26 06:33:42 PDT 2019

SureYeaah marked 12 inline comments as done.
SureYeaah 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
----------------
kadircet wrote:
> 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 ...;
> }
> ```
Aah, I didn't know you could declare a class inside a function. Nice.

================
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) {
----------------
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?

================
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) {
----------------
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?

================
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
----------------
kadircet wrote:
> 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 ?
This check is only for extraction from the for(<Init>; <Cond>; <Inc>) and not the body of the for. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773