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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Please forgive the mix of high-level and low-level comments here.
Happy to discuss further of course!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:55
+// checks whether any variable in a given expr is declared in the DeclStmt
+static bool isDeclaredIn(const DeclStmt *Decl, const Expr *Exp,
+                         const SourceManager &M) {
----------------
nit: why take a DeclStmt instead of just the Decl here?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:55
+// checks whether any variable in a given expr is declared in the DeclStmt
+static bool isDeclaredIn(const DeclStmt *Decl, const Expr *Exp,
+                         const SourceManager &M) {
----------------
sammccall wrote:
> nit: why take a DeclStmt instead of just the Decl here?
This function name is confusing - it's unclear which of DeclStmt vs Exp is declared in which, and that it's subexpressions of Exp that are being checked

maybe `bool referencesLocalDecls`? and rename Decl -> Scope, as it provides the meaning of "local"


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:56
+static bool isDeclaredIn(const DeclStmt *Decl, const Expr *Exp,
+                         const SourceManager &M) {
+
----------------
nit: SourceManager is almost universally `SM` (or something more verbose)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:62
+  public:
+    std::vector<DeclRefExpr *> DeclRefExprs;
+    bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { // NOLINT
----------------
nit: seems a little clearer to collect the referenced decls rather than the referencing exprs.
(It will also generalize better if we want to do things like notice referenced local classes, which we don't yet)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:73
+    // Beginning location of the ValueDecl of the DeclRef
+    auto ValueDeclLoc = DeclRef->getDecl()->getBeginLoc();
+    // return true if this ValueDecl location is within the DeclStmt
----------------
nit: "ValueDecl" is not usually a very interesting class conceptually, I'd just call this DeclLoc


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:73
+    // Beginning location of the ValueDecl of the DeclRef
+    auto ValueDeclLoc = DeclRef->getDecl()->getBeginLoc();
+    // return true if this ValueDecl location is within the DeclStmt
----------------
sammccall wrote:
> nit: "ValueDecl" is not usually a very interesting class conceptually, I'd just call this DeclLoc
throughout you're using `auto` a bit too heavily - usually doesn't aid readability when the type is a single word like `SourceLocation` here and isn't named in the expression


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:130
+// TODO: Add more unit tests after SelectionTree ancestor bug is fixed
+static bool extractionAllowed(const Stmt *ParStmt, const SelectionTree::Node *N,
+                              const SourceManager &M) {
----------------
Another general style point: LLVM does use short variable names (like `N`) sometimes when the type unambiguously describes the role of the variable and the scope is small.

That's not the case here: a `SelectionTree::Node` could be almost anything so I'd suggest TargetExpr or so.

Conversely I think `Parent` or `NewParent` would be clearer than `ParStmt` which repeats more info from the type.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:132
+                              const SourceManager &M) {
+  if (isa<ForStmt>(ParStmt) &&
+      !checkForStmt(dyn_cast<ForStmt>(ParStmt), N->ASTNode.get<Expr>(), M))
----------------
I might be missing something, but as I understand:
 - you're proposing moving the expr N out of the scope ParStmt
 - you're testing whether any subexpression of N references anything declared in certain substmts of ParStmt
 - because of "certain substmts" you're only supporting this for some types of ParStmt

why can't we just check for anything declared inside ParStmt, and make this fully general (applied for all stmts)?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:157
+// we also check if doing the extraction will take a variable out of scope
+static const clang::Stmt *getStmt(const SelectionTree::Node *N,
+                                  const SourceManager &M) {
----------------
this needs a much more descriptive name.
maybe `getInsertionPoint()`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:159
+                                  const SourceManager &M) {
+  auto CurN = N;
+  while (CurN->Parent) {
----------------
auto* (or `const auto*`) - we typically spell out the pointer-ness


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:160
+  auto CurN = N;
+  while (CurN->Parent) {
+    auto ParStmt = CurN->Parent->ASTNode.get<Stmt>();
----------------
please make this a for loop, you have an initializer, condition and increment


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:162
+    auto ParStmt = CurN->Parent->ASTNode.get<Stmt>();
+    if (ParStmt) {
+      if (isa<CompoundStmt>(ParStmt)) {
----------------
so there's a few cases for CurN here:
 - Expr
 - non-Expr Stmts
 - Decl
 - other weird things (TypeLoc, template parameter list, ...)

As a first cut I think we should be a bit more systematic and conservative about the cases where we allow a variable to be inserted far away from the extraction.

I'd suggest that for Expr your "default" behavior is to keep traversing up, for Decl you probably want to stop and give up and similarly for the "everything else" case you want to give up. Maybe there are cases where these are OK/important, but we should probably whitelist those.

For non-Expr Stmts, you're currently blacklisting CaseStmt for traversal through, I think this should be a whitelist instead, just allowing loops for now.

I think this obviates the need for `needBraces` and `isNodeInside` for now, unless I'm missing something.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:173
+      // give up if extraction will take a variable out of scope
+      if (!extractionAllowed(ParStmt, N, M))
+        break;
----------------
here you're traversing the whole Expr to find the referenced decls at each iteration of this loop.
Can you analyse the expr just once, and reuse the list of decls?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:186
+      toHalfOpenFileRange(M, Ctx.getLangOpts(), Exp->getSourceRange());
+  auto ExpCode = toSourceCode(M, *ExpRng);
+  return tooling::Replacement(M, ExpRng->getBegin(), ExpCode.size(), VarName);
----------------
no need to get the actual source code: length is `SM.getFileOffset(ExpRng->End) - SM.getFileOffset(ExpRng->Begin)`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:186
+      toHalfOpenFileRange(M, Ctx.getLangOpts(), Exp->getSourceRange());
+  auto ExpCode = toSourceCode(M, *ExpRng);
+  return tooling::Replacement(M, ExpRng->getBegin(), ExpCode.size(), VarName);
----------------
sammccall wrote:
> no need to get the actual source code: length is `SM.getFileOffset(ExpRng->End) - SM.getFileOffset(ExpRng->Begin)`
I think you should maybe handle the case where ExpRng is none (e.g. because something is unexpectedly inside a macro).

If you're sure this can't happen, add an assert with a message.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:202
+      toHalfOpenFileRange(M, Ctx.getLangOpts(), Stm->getSourceRange());
+  // TODO: Replace auto with explicit type
+  std::string ExtractedVarDecl =
----------------
also may need to use & or && as appropriate. FIXME is fine for this too.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:208
+
+// FIXME(Bug in selection tree): doesn't work for int a = 6, b = 8;
+// FIXME: case constant refactoring
----------------
fix for this has landed


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:221
+    return false;
+  return (Stm = getStmt(N, M));
+}
----------------
this is clever and not really a common idiom in LLVM code, so a bit hard to decipher

usually prefer `Stm = ...; return Stm != nullptr;`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:236
+std::string ExtractVariable::title() const {
+  return "Extract to dummy variable";
+}
----------------
I'm not sure "dummy" is very meaningful to the user here.
"Extract to variable" or "Extract subexpression to variable"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773