SureYeaah added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:210
+// - we don't look inside macro expansions in the subexpressions
+// - we only adjust the extracted range, so references in the unselected parts
+// of the AST expression (e.g. `a`) are still considered referenced for
----------------
A related FIXME would be helpful since it's pretty easy to fix(I think?) and
seems like something important.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:213
+// the purposes of calculating the insertion point.
+namespace {
+
----------------
Why do we need an anon namespace inside another anon namespace?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:217
+// It can represent either an overloaded or built-in operator.
+struct ParsedBinaryOperator {
+ BinaryOperatorKind Kind;
----------------
Wouldn't it make more sense this to have this in some other place (maybe
Selection) because other refactorings might want to make use of this feature?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:235
+ N.ASTNode.get<Expr>())) {
+ if (Op->isInfixBinaryOp()) {
+ Kind = BinaryOperator::getOverloadedOpcode(Op->getOperator());
----------------
Can we negate this if to reduce nesting?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:283
+const SelectionTree::Node *getEndpoint(const SelectionTree::Node &N,
+ BinaryOperatorKind OuterOp, bool
IsStart,
+ const SourceManager &SM) {
----------------
We're considering the case where all the operators are the same. Isn't it true
that in that case, the End will be the the first 'selected RHS' that we find
during the traversal? If so, can't we just get rid of the IsStart?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:393
std::string VarName = "dummy";
+ SourceRange Range = Target->getExtractionChars();
// insert new variable declaration
----------------
Why not just compute this inside ExtractionContext?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65139/new/
https://reviews.llvm.org/D65139
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
