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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, this looks right to me.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:338
+  const CompoundStmt *EnclosingScope = ParentNode->get<CompoundStmt>();
+  if (const auto *If = ParentNode->get<IfStmt>())
+    if (const auto *Then = dyn_cast<CompoundStmt>(If->getThen()))
----------------
I found that the current structure (we have two places of this `If` cases, and they are separated) is a bit hard to follow, I'd suggest union them, just like the code below, it seems straight-forward to me, and easy to extend. 


```
if (const auto* CompoundStmtEnclosing = Parents.begin()->get<CompoundStmt>()) {
   CheckCompoundStmt(CompoundStmtEnclosing);
   // below we detect conflicts in conditions or others (if the CompoundStmt is a function body, we could detect the parameter decls as well)
   getParents(CompoundStmtEnclosing);
   if ( ...<IfStmt>()) {
      CheckConditionVariable();
   } else if (...) {
   }
} else if (const auto* IfEnclosing = ... <IfStmt>()) {
   CheckCompoundStmt(If->getThen());
  // possibly CheckCompoundStmt(If->getElse)
} else {
  ...
}
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:339
+  if (const auto *If = ParentNode->get<IfStmt>())
+    if (const auto *Then = dyn_cast<CompoundStmt>(If->getThen()))
+      EnclosingScope = Then;
----------------
thinking more about the `if` case, I think else should be included as well? no need to address in this patch.

like

```
if (int a = 0) {
} else {
  int s; // rename s=>a will cause a compiling error.
}
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:352
+    for (const auto &Child : DS->getDeclGroup())
+      if (const auto *VD = dyn_cast<VarDecl>(Child))
+        if (VD != &RenamedDecl && VD->getName() == NewName)
----------------
nit: VarDecl => NamedDecl


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95925/new/

https://reviews.llvm.org/D95925