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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Comments re the `Extract` class.
It seems OK to me, i'm not really sure whether it's an improvement or not after the pieces (needsBraces, checForStmt etc) are removed. Up to you.

(As the previous comments would reduce the scope of the patch, it'd be nice to address those before reviewing further).



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:33
+
+// class to store information about the Expr that is being extracted
+class Extract {
----------------
drop "class to store" from the comment


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:33
+
+// class to store information about the Expr that is being extracted
+class Extract {
----------------
sammccall wrote:
> drop "class to store" from the comment
this class needs to have a clearer name and purpose.

`ExtractionContext` is marginally better, maybe there's a more specific unifying concept to be found.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:41
+  const clang::Expr *getExpr() const;
+  const SelectionTree::Node *getNode() const;
+  bool needBraces(const Stmt *InsertionPoint, const SourceManager &SM) const;
----------------
Many of these names should be a little more specific about their semantics.
"Node" is the type, and isn't specific enough to know which node it is. `getExprNode()`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:50
+  std::vector<clang::Decl *> ReferencedDecls;
+  std::vector<clang::Decl *> getReferencedDecls();
+  bool checkForStmt(const ForStmt *F, const SourceManager &SM) const;
----------------
probably call this computeReferencedDecls()? (or as Kadir noted, pull out of the class)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:54
+};
+Extract::Extract(const clang::Expr *Expr, const SelectionTree::Node *Node)
+    : Expr(Expr), Node(Node) {
----------------
please inline trivial functions like this, getExpr(), getNode()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63773