[PATCH] D95925: [clangd] Detect rename conflicits within enclosing scope

[Haojian Wu via Phabricator via cfe-commits]

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