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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:42
+  // Generate Replacement for replacing selected expression with given VarName
+  tooling::Replacement replaceWithVar(std::string VarName) const;
+  // Generate Replacement for declaring the selected Expr as a new variable
----------------
nit: StringRef almost always for params you don't mutate


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:46
+  // returns true if the Expr can be extracted
+  inline bool isExtractable() const;
+
----------------
define trivial (and even simple) methods inline


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:65
+};
+/// Extracts an expression to the variable dummy
+/// Before:
----------------
nit: can you move the tweak to the end, to avoid alternating between the inner logic implementation and the outer interface?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:88
+// this is to avoid the function name being extracted in a function call.
+// e.g. int a = [[f]]();
+bool isAFunctionRef(const clang::Expr *Expr) {
----------------
I'd suggest naming this something more after the intent than after the structure, as we may want to refine it over time, e.g. `isTooTrivialToExtract` or `isEligibleToExtract`


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


================
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()))
----------------
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)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+  const clang::DeclRefExpr *DeclRef = dyn_cast_or_null<DeclRefExpr>(Expr);
+  if (DeclRef && isa<FunctionDecl>(DeclRef->getDecl()))
+    return true;
----------------
you may want to check whether this does the right thing for a function template


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:120
+  if ((Expr = Node->ASTNode.get<clang::Expr>())) {
+    Expr->dump();
+    ReferencedDecls = computeReferencedDecls(Expr);
----------------
remove debug dumping


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:122
+    ReferencedDecls = computeReferencedDecls(Expr);
+    Extractable =
+        !isAFunctionRef(Expr) && (InsertionPoint = computeInsertionPoint());
----------------
Can we be a bit more explicit about the control flow and boolean initialization here?
e.g. `if (isAFunctionRef(Expr)) return; if (InsertionPoint = computeInsertionPoint()) Extractable = true;`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:135
+// variable out of scope
+bool ExtractionContext::canInsertBefore(const clang::Stmt *Scope) const {
+  if (!Scope)
----------------
The name doesn't really match the behavior here - there are lots of other reasons we couldn't insert before a point.
this sounds more like `ExtractionContext::exprIsValidOutside` or something?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137
+  if (!Scope)
+    return true;
+  SourceLocation ScopeBegin = Scope->getBeginLoc();
----------------
this behavior is not at all obvious at the callsite. I'd suggest making the caller check for null, and taking a reference.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:158
+const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
+  bool WaitForSwitch = false;
+  for (const SelectionTree::Node *CurNode = getExprNode(); CurNode->Parent;
----------------
The scope of this patch is growing in ways that don't seem to be related to the review.

Can you revert the switch-related changes and move them to another patch?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:165
+      break;
+    if (const clang::Stmt *Ancestor = CurNode->Parent->ASTNode.get<Stmt>()) {
+      if (WaitForSwitch) {
----------------
this is the Parent, not an Ancestor, I think?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:173
+      if (isa<CompoundStmt>(Ancestor)) {
+        // Return if the CurInsertionPoint is not inside a macro. Else we
+        // continue traversing up.
----------------
this describes what the code is doing, but that's fairly obvious from the code. It would be useful to say why instead.
e.g. "We only allow insertion inside a CompoundStmt. It must be written in the file, not a macro."


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:178
+        return CurInsertionPoint;
+      } else if (isa<SwitchCase>(Ancestor))
+        WaitForSwitch = true;
----------------
nit: please use braces consistently within an else-if chain


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:181
+      // don't extract from lambda captures/default arguments
+      else if (isa<LambdaExpr>(Ancestor))
+        break;
----------------
I think this is also new in this revision.
Can we move it to a followup patch? I'm not sure exactly what you're trying to do, but suspect there's a better way/place for it.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:183
+        break;
+    }
+    // don't extract from function/method default arguments.
----------------
you're still traversing upwards through arbitrary AST nodes with a blacklist - as discussed previously this doesn't seem safe


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:219
+
+// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a = 1
+// FIXME: Expressions involving macro expansions
----------------
this FIXME probably belongs on the `isEligibleToExtract` or similar function


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:220
+// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a = 1
+// FIXME: Expressions involving macro expansions
+bool ExtractVariable::prepare(const Selection &Inputs) {
----------------
it's not clear what the bug or fix is for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773