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