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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:34
+// class to store information about the Expr that is being extracted
+class Extract {
+public:
----------------
most of the methods seem to be taking a SM, why not make it a class field ?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:47
+private:
+  const clang::Expr *Expr;
+  const SelectionTree::Node *Node;
----------------
maybe some comments about the fields/methods below?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:71
+}
+std::vector<clang::Decl *> Extract::getReferencedDecls() {
+  // RAV subclass to find all DeclRefs in a given Stmt
----------------
this method doesn't use class state, maybe move it out-of-class to a function?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:163
+  // the expression to extract
+  const Extract *Target = nullptr;
+  // the statement before which variable will be extracted to
----------------
store a unique_ptr instead


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:186
+// we also check if doing the extraction will take a variable out of scope
+static const clang::Stmt *getInsertionPoint(const Extract *Target,
+                                            const SourceManager &SM) {
----------------
this seems to be using a lot of class state(and some functions are exposed merely for use of this function). why not make this one a class method instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773