sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
I think we should go ahead and land this. The only points that I'd really like
to see fixed is `shared_ptr`, mostly because it's a strong signal there's
something complicated going on and I don't think (or hope) there is!
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+ return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};
----------------
sammccall wrote:
> use `printType` from AST.h?
>
> (You'll want to drop qualifiers/refs before calling that, but it's not at all
> obvious from the function name here that they're dropped, so that should be
> at the callsite anyway)
Second half is not done: the suggestion is that it's really confusing where
spellType is called that it drops qualifiers, so just I'd inline this into the
callsite.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+ if (WithTypeAndQualifiers) {
+ if (IsConst)
+ Result += "const ";
----------------
SureYeaah wrote:
> 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.
I think the point is that the QualType in parameter could/should represent the
*parameter* type, not the type of the variable being captured.
e.g. it could be `const std::string&` even if the original variable was just
`std::string`.
This seems the more natural model (the struct is called Parameter, not
CapturedVariable or Argument) but there may be reasons not to do this.
================
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?
shared ownership is less common and harder to reason about than exclusive
ownership
Generally we prefer deciding where ownership should reside (T or Optional<T> or
unique_ptr<T>), and where you should have an unowned reference (T& or T*).
In this case, findExtractionZone() should ideally return
`Optional<ExtractionZone>`, after prepare() the ExtractFunction class should
own it (as a Optional<ExtractionZone>), and apply should pass it around as a
const ExtractionZone&.
If it turns out ExtractionZone isn't a movable type, then
`Optional<ExtractionZone>` won't work and you'll need
`unique_ptr<ExtractionZone>` instead. But shared_ptr shouldn't be needed
regardless.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:119
+ case SelectionTree::Selection::Partial:
+ // Treat Partially selected VarDecl as completely selected since
+ // SelectionTree doesn't always select VarDecls correctly.
----------------
D66872 has the fix if you'd rather avoid these workarounds
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+ SourceRange EnclosingFuncRange;
+ std::set<const Stmt *> RootStmts;
+ SourceLocation getInsertionPoint() const {
----------------
llvm::DenseSet<const Stmt*>
std::set is a pretty terrible data structure unless you really really need
order.
(Lots of allocations, lots of pointer-chasing)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:238
+ // Convert RootStmt Nodes to Stmts and insert in RootStmts.
+ llvm::transform(ExtZone->Parent->Children,
+ std::inserter(ExtZone->RootStmts,
ExtZone->RootStmts.begin()),
----------------
this is also:
```
for (const SelectionTree::Node *N)
ExtZone->RootStmts.insert(N->ASTNode.get<Stmt>());
```
which is shorter and (I think) less obfuscated
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65526/new/
https://reviews.llvm.org/D65526
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
