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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome! Do you have commit access?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:90
+bool isAFunctionRef(const clang::Expr *Expr) {
+  const clang::DeclRefExpr *DeclRef = dyn_cast_or_null<DeclRefExpr>(Expr);
+  if (DeclRef && isa<FunctionDecl>(DeclRef->getDecl()))
----------------
SureYeaah wrote:
> sammccall wrote:
> > sammccall wrote:
> > > Extracting just a declrefexpr (and replacing it with another declrefexpr) doesn't seem useful. Any reason to only do this for functions, rather than all declrefexprs?
> > a syntactically similar case is MemberExpr where isImplicitAccess() is true. (This is referring to a method of the current class, without qualification)
> Extracting
>   int a = [[f]]();
> yields
>   auto dummy = f;
>   int a = dummy();
> 
> I thought this should be prevented. But now I see there's no need to do that.
> I'll add a triviality check if needed in the next patch.
sure - I just mean `int a = b;` -> `auto dummy = b; int a = dummy;` is equally pointless, so I wasn't sure about the function check. But fine to defer.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:37
+                    const ASTContext &Ctx);
+  // return the expr
+  const clang::Expr *getExpr() const { return Expr; }
----------------
nit: drop comments that just echo the name, like this one


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:82
+
+// An expr is not extractable if it's null, a delete expression or a comma
+// separated expression
----------------
why a delete expression? if this generalises to "expression of type void" then you can check for that directly


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:101
+    ReferencedDecls = computeReferencedDecls(Expr);
+    if ((InsertionPoint = computeInsertionPoint()))
+      Extractable = true;
----------------
nit: remove extra parens


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:107
+// checks whether extracting before InsertionPoint will take a
+// variable out of scope
+bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
----------------
nit: a variable reference out of scope


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:134
+    if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get<clang::Stmt>()) {
+      // Allow all expressions except LambdaExpr
+      if (isa<clang::Expr>(Stmt))
----------------
why?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137
+        return !isa<LambdaExpr>(Stmt);
+      return isa<AttributedStmt>(Stmt) || isa<CompoundStmt>(Stmt) ||
+             isa<DeclStmt>(Stmt) || isa<DoStmt>(Stmt) || isa<ForStmt>(Stmt) ||
----------------
can you add a comment explaining approximately why many (most) stmts are allowed but some others cause problems?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+  std::string ExtractedVarDecl = std::string("auto ") + VarName.str() + " = " +
+                                 ExtractionCode.str() + "; ";
+  return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
----------------
nit: trailing space? should this be a newline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773