hokein added a comment.
thanks, looks better now, just some nits to improve the code readability.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:332
+ DynTypedNodeList Parents = Ctx.getParents(RenamedDecl);
+ if (Parents.size() != 1 || !Parents.begin()->get<DeclStmt>())
+ return nullptr;
----------------
since we repeat this code multiple times, I think we can use wrap it with a
lambda (e.g. getSingleParent etc).
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:338
+ // This helper checks if any statement within DeclStmt has NewName.
+ auto CheckDeclStmt = [&](const DeclStmt *DS) -> const NamedDecl * {
+ for (const auto &Child : DS->getDeclGroup())
----------------
maybe name it `LookupInDeclStmt` and pass the `Name` as a parameter, the same
below.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:345
+ };
+ auto CheckCompoundStmt = [&](const CompoundStmt *CS) -> const NamedDecl * {
+ if (!CS)
----------------
the same lookupInCompoundStmt.
nit: I would pass Stmt directly, and do the "compoundStmt" cast internally, to
save some boilerplate cast in call sides :)
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:350
+ if (const auto *DS = dyn_cast<DeclStmt>(Node))
+ if (const auto *Result = CheckDeclStmt(DS))
+ return Result;
----------------
nit: can be just `return CheckDeclStmt(DS);`
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:355
+ // This helper checks if there is a condition variable has NewName.
+ auto CheckConditionVariable = [&](const auto *Scope) -> const NamedDecl * {
+ if (!Scope)
----------------
nit: explicit spell out the `auto` type?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:359
+ if (const auto *ConditionDS = Scope->getConditionVariableDeclStmt())
+ if (const auto *Result = CheckDeclStmt(ConditionDS))
+ return Result;
----------------
nit: the same here, `return CheckDeclStmt
(Scope->getConditionVariableDeclStmt())`, just adding the nullptr handling in
`CheckDeclStmt`.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:364
+ const auto *ParentNode = Parents.begin();
+ if (const auto *EnclosingCS = ParentNode->get<CompoundStmt>()) {
+ if (const auto *Result = CheckCompoundStmt(EnclosingCS))
----------------
I understand all cases of these if branches at the moment, but I think we'd
better to add some comments, or examples here -- help us understand this code
in case that we forget the context in the future :)
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:382
+ if (Parameter->getName() == NewName)
+ return Parameter;
+ }
----------------
I think we need a `return null` here (or use if-else pattern), otherwise, the
following if-stmt will be examined as well, which seems no necessary.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95925/new/
https://reviews.llvm.org/D95925
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
