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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
