sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Awesome! Do you have commit access?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:90
+bool isAFunctionRef(const clang::Expr *Expr) {
+ const clang::DeclRefExpr *DeclRef = dyn_cast_or_null<DeclRefExpr>(Expr);
+ if (DeclRef && isa<FunctionDecl>(DeclRef->getDecl()))
----------------
SureYeaah wrote:
> sammccall wrote:
> > sammccall wrote:
> > > Extracting just a declrefexpr (and replacing it with another declrefexpr)
> > > doesn't seem useful. Any reason to only do this for functions, rather
> > > than all declrefexprs?
> > a syntactically similar case is MemberExpr where isImplicitAccess() is
> > true. (This is referring to a method of the current class, without
> > qualification)
> Extracting
> int a = [[f]]();
> yields
> auto dummy = f;
> int a = dummy();
>
> I thought this should be prevented. But now I see there's no need to do that.
> I'll add a triviality check if needed in the next patch.
sure - I just mean `int a = b;` -> `auto dummy = b; int a = dummy;` is equally
pointless, so I wasn't sure about the function check. But fine to defer.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:37
+ const ASTContext &Ctx);
+ // return the expr
+ const clang::Expr *getExpr() const { return Expr; }
----------------
nit: drop comments that just echo the name, like this one
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:82
+
+// An expr is not extractable if it's null, a delete expression or a comma
+// separated expression
----------------
why a delete expression? if this generalises to "expression of type void" then
you can check for that directly
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:101
+ ReferencedDecls = computeReferencedDecls(Expr);
+ if ((InsertionPoint = computeInsertionPoint()))
+ Extractable = true;
----------------
nit: remove extra parens
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:107
+// checks whether extracting before InsertionPoint will take a
+// variable out of scope
+bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
----------------
nit: a variable reference out of scope
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:134
+ if (const clang::Stmt *Stmt = InsertionPoint->ASTNode.get<clang::Stmt>()) {
+ // Allow all expressions except LambdaExpr
+ if (isa<clang::Expr>(Stmt))
----------------
why?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137
+ return !isa<LambdaExpr>(Stmt);
+ return isa<AttributedStmt>(Stmt) || isa<CompoundStmt>(Stmt) ||
+ isa<DeclStmt>(Stmt) || isa<DoStmt>(Stmt) || isa<ForStmt>(Stmt) ||
----------------
can you add a comment explaining approximately why many (most) stmts are
allowed but some others cause problems?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+ std::string ExtractedVarDecl = std::string("auto ") + VarName.str() + " = " +
+ ExtractionCode.str() + "; ";
+ return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
----------------
nit: trailing space? should this be a newline?
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
