kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:54 + +// RAV subclass to find all DeclRefs in a given Stmt +class FindDeclRefsVisitor ---------------- I believe this class is rather used to check if any decl referenced in an expression was declared in a statement, rather then finding those declrefs. So what about turning it inside out :D ? Something like: ``` bool isDeclaredIn(const DeclStmt *DS, const Expr *E, SourceManager &SM) { // Collects all DelcRefs in an AST node. class FindDeclRefsVisitor .... { public: bool VisitDeclRefExpr(... *DRE) { DeclRefs.push_back(DRE); return true; } std::vector<DelcRefExpr*> DeclRefs; }; FindDeclRefsVisitor X; X.TraverseStmt ... for(auto *DRE : X.DeclRefs) { ... } return ...; } ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:80 + +// Here T is a Stmt subclass and getBody is a member function that returns its +// "body" Stmt. ---------------- looks really nice but we already type all of the member fields and types in the code explicitly anyway, maybe simplify a bit with: ``` static bool needBraces(....) { auto IsNodeInside = [..](SourceRange Body) { return M.isPointWithin(...); }; if(ForStmt *Stmt = llvm::dyn_cast_or_null<ForStmt>(Stm)) return IsNodeInside(Stmt->getBody()); ... ... } ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:95 +// Returns true if we will need braces after extraction +static bool needBraces(const SelectionTree::Node *N, const Stmt *Stm, + const SourceManager &M) { ---------------- I suppose currently this check is not doing what it claims to do(not checking for whether it is the only statement and/or if there are enclosing braces). Maybe leave it out of this patch completely? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:108 +// check whether to allow extraction from for(...) +static bool checkFor(const ForStmt *F, const Expr *Exp, + const SourceManager &M) { ---------------- I believe it would be better to have a more generic check that will ensure all of the delcs referenced in `Exp` are still visible in the context which we'll insert the new statement. WDYT? Were there any reasons to make it specific for `ForStmt` ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:119 +// Return the Stmt before which we need to insert the extraction. +static const clang::Stmt *getStmt(const SelectionTree::Node *N, + const SourceManager &M) { ---------------- could you add a few comments explaining the logic? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137 + N->ASTNode.get<Expr>(), M)) { + // Check whether the expression references any variable in the for + // initializer and if so, we can't extract ---------------- what about a case like: ``` for(int i=0;;) { int z = [[f(i)*10]] + 5; } ``` I believe it should be OK to extract in this case right ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:177 + // insert new variable declaration + if (auto Err = Result.add(tooling::Replacement( + Ctx.getSourceManager(), StmRng->getBegin(), 0, ExtractedVarDecl))) ---------------- maybe extract these into a function that will take `VarName`, `Stm` and `Exp` as parameters and will return an `Expected<Replacement>`. So that you can easily change "dummy" part later on if we get another way to invoke this functionality. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:181 + // replace expression with variable name + if (auto Err = Result.add(tooling::Replacement( + Ctx.getSourceManager(), ExpRng->getBegin(), ExpCode.size(), "dummy"))) ---------------- same as above 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