kadircet marked an inline comment as done. kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165 + else + SR.setEnd(ChildFileRange->getEnd()); + } ---------------- SureYeaah wrote: > kadircet wrote: > > I suppose this relies on the fact that "AST contains the nodes ordered by > > their begin location"? Could you add an assertion for that? > I've removed the loop, should I still add this? no need ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225 +// enclosingFunction. +std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc, + const SourceManager &SM, ---------------- SureYeaah wrote: > SureYeaah wrote: > > sammccall wrote: > > > kadircet wrote: > > > > why is this function returning a shared_ptr ? > > > avoid shared_ptr unless there's a strong reason. > > > `Optional<ExtractionZone>` seems fine here? > > And store a unique_ptr to the optional in ExtractFunction? > Because ExtractFunction needs to store a pointer/reference to ExtractionZone > somehow in prepare and access it in apply. Let's figure it out in the `ExtractFunction` this can still return an `Optional<ExtractionZone>` which you can convert into a pointer/reference later on if need be? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547 +private: + std::shared_ptr<ExtractionZone> ExtZone; +}; ---------------- SureYeaah wrote: > kadircet wrote: > > why do you need a shared_ptr here? > getExtractionZone creates an Optional<ExtractionZone> which needs to persist > from prepare to apply. Is there a better way to store a reference to the > ExtractionZone instance inside ExtractFunction? Then why not just store `Optional<ExtractionZone>` ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146 + SourceRange EnclosingFuncRange; + std::set<const Stmt *> RootStmts; + SourceLocation getInsertionPoint() const { ---------------- lifetime of this field looks complicated can you add some comments on how/when it is initialized and maybe enforce immutability ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150 + } + bool isRootStmt(const Stmt *S) const; + // The last root statement is important to decide where we need to insert a ---------------- it seems like you are rather using it to decide whether a statement is inside the zone or not? Could you rather rename it to reflect that and add some comments? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:169 + // FIXME: Support extraction from methods. + if (CurNode->ASTNode.get<CXXMethodDecl>()) + return nullptr; ---------------- nit: ``` if (isa<CXXMethodDecl>(Func)) ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172 + // FIXME: Support extraction from templated functions. + if (CurNode->Parent->ASTNode.get<FunctionTemplateDecl>()) + return nullptr; ---------------- nit: ``` if(isa<FunctionTemplateDecl>(Func)) ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346 + ZoneRelative DeclaredIn; + // index of the declaration or first reference + unsigned DeclIndex; ---------------- i think you mean `index of the first reference to this decl` ? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:486 + // (this includes the case of recursive call to EnclosingFunc in Zone). + if (!VD || dyn_cast_or_null<FunctionDecl>(DeclInfo.TheDecl)) + return false; ---------------- nit `isa<FunctionDecl>` instead of `dyn_cast` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65526/new/ https://reviews.llvm.org/D65526 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits