SureYeaah added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+ else
+ SR.setEnd(ChildFileRange->getEnd());
+ }
----------------
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?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+ for (const Node *Child : Parent->Children) {
----------------
sammccall wrote:
> kadircet wrote:
> > `hasOnlyRootStmtsAsChildren` ?
> nit: I think this would be clearer as
> `canBeRootStmt(const Node*)` and write the callsite as
> `llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though
Replaced with isRootStmt
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr<ExtractionZone> getExtractionZone(const Node *CommonAnc,
+ const SourceManager &SM,
----------------
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?
================
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:
> 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.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+ NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+ tooling::ExtractionSemicolonPolicy SemicolonPolicy,
----------------
sammccall wrote:
> this is just initializing public fields, drop the constructor?
> (The callsite is clearer here if you initialize them by name)
`ExtractionSemicolonPolicy` doesn't have a default constructor.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+ if (WithTypeAndQualifiers) {
+ if (IsConst)
+ Result += "const ";
----------------
kadircet wrote:
> why don't we infer this const from QualType ?
In a future patch we will want to use heuristics like has the variable been
assigned, was it passed as a non-const reference, was its address taken, etc.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+ std::shared_ptr<ExtractionZone> ExtZone;
+};
----------------
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?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:611
+ EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+ // Expressions aren't extracted.
+ EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
----------------
sammccall wrote:
> wait, what?
> expressions that *aren't statements* shouldn't be extracted.
> But what's wrong with this one?
> (Many useful statements are expressions, such as function calls)
For now we don't extract if the selection is an expression. I've added a fixme
to change that to sub-expressions only.
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
