sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:42
+ // Generate Replacement for replacing selected expression with given VarName
+ tooling::Replacement replaceWithVar(std::string VarName) const;
+ // Generate Replacement for declaring the selected Expr as a new variable
----------------
nit: StringRef almost always for params you don't mutate
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:46
+ // returns true if the Expr can be extracted
+ inline bool isExtractable() const;
+
----------------
define trivial (and even simple) methods inline
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:65
+};
+/// Extracts an expression to the variable dummy
+/// Before:
----------------
nit: can you move the tweak to the end, to avoid alternating between the inner
logic implementation and the outer interface?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:88
+// this is to avoid the function name being extracted in a function call.
+// e.g. int a = [[f]]();
+bool isAFunctionRef(const clang::Expr *Expr) {
----------------
I'd suggest naming this something more after the intent than after the
structure, as we may want to refine it over time, e.g. `isTooTrivialToExtract`
or `isEligibleToExtract`
================
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()))
----------------
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?
================
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()))
----------------
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)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:91
+ const clang::DeclRefExpr *DeclRef = dyn_cast_or_null<DeclRefExpr>(Expr);
+ if (DeclRef && isa<FunctionDecl>(DeclRef->getDecl()))
+ return true;
----------------
you may want to check whether this does the right thing for a function template
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:120
+ if ((Expr = Node->ASTNode.get<clang::Expr>())) {
+ Expr->dump();
+ ReferencedDecls = computeReferencedDecls(Expr);
----------------
remove debug dumping
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:122
+ ReferencedDecls = computeReferencedDecls(Expr);
+ Extractable =
+ !isAFunctionRef(Expr) && (InsertionPoint = computeInsertionPoint());
----------------
Can we be a bit more explicit about the control flow and boolean initialization
here?
e.g. `if (isAFunctionRef(Expr)) return; if (InsertionPoint =
computeInsertionPoint()) Extractable = true;`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:135
+// variable out of scope
+bool ExtractionContext::canInsertBefore(const clang::Stmt *Scope) const {
+ if (!Scope)
----------------
The name doesn't really match the behavior here - there are lots of other
reasons we couldn't insert before a point.
this sounds more like `ExtractionContext::exprIsValidOutside` or something?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:137
+ if (!Scope)
+ return true;
+ SourceLocation ScopeBegin = Scope->getBeginLoc();
----------------
this behavior is not at all obvious at the callsite. I'd suggest making the
caller check for null, and taking a reference.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:158
+const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
+ bool WaitForSwitch = false;
+ for (const SelectionTree::Node *CurNode = getExprNode(); CurNode->Parent;
----------------
The scope of this patch is growing in ways that don't seem to be related to the
review.
Can you revert the switch-related changes and move them to another patch?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:165
+ break;
+ if (const clang::Stmt *Ancestor = CurNode->Parent->ASTNode.get<Stmt>()) {
+ if (WaitForSwitch) {
----------------
this is the Parent, not an Ancestor, I think?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:173
+ if (isa<CompoundStmt>(Ancestor)) {
+ // Return if the CurInsertionPoint is not inside a macro. Else we
+ // continue traversing up.
----------------
this describes what the code is doing, but that's fairly obvious from the code.
It would be useful to say why instead.
e.g. "We only allow insertion inside a CompoundStmt. It must be written in the
file, not a macro."
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:178
+ return CurInsertionPoint;
+ } else if (isa<SwitchCase>(Ancestor))
+ WaitForSwitch = true;
----------------
nit: please use braces consistently within an else-if chain
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:181
+ // don't extract from lambda captures/default arguments
+ else if (isa<LambdaExpr>(Ancestor))
+ break;
----------------
I think this is also new in this revision.
Can we move it to a followup patch? I'm not sure exactly what you're trying to
do, but suspect there's a better way/place for it.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:183
+ break;
+ }
+ // don't extract from function/method default arguments.
----------------
you're still traversing upwards through arbitrary AST nodes with a blacklist -
as discussed previously this doesn't seem safe
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:219
+
+// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a = 1
+// FIXME: Expressions involving macro expansions
----------------
this FIXME probably belongs on the `isEligibleToExtract` or similar function
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:220
+// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a = 1
+// FIXME: Expressions involving macro expansions
+bool ExtractVariable::prepare(const Selection &Inputs) {
----------------
it's not clear what the bug or fix is for this
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
