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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:333
+  // not invalidated.
+  DynTypedNodeList Parents(DynTypedNode::create(RenamedDecl));
+  auto GetSingleParent = [&](DynTypedNode Node) -> const DynTypedNode * {
----------------
If the intention is for storage, maybe call it Storage.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:334
+  DynTypedNodeList Parents(DynTypedNode::create(RenamedDecl));
+  auto GetSingleParent = [&](DynTypedNode Node) -> const DynTypedNode * {
+    Parents = Ctx.getParents(Node);
----------------
`const DynTypedNode &`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:362
+      for (const auto *Node : CS->children())
+        return CheckDeclStmt(dyn_cast<DeclStmt>(Node), Name);
+    return nullptr;
----------------
I think we need to iterate *all* children, rather than the first one. looks like our testcase doesn't cover this, maybe add one.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:372
+
+  // CompoundStmt is the most common enclosing scope. In the simplest case we
+  // just iterate through sibling DeclStmts and check for collisions.
----------------
 CompoundStmt is the most common enclosing scope for function-local symbols.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:378
+    const auto *ScopeParent =
+        GetSingleParent(DynTypedNode::create(*EnclosingCS));
+    // CompoundStmt may be found within if/while/for. In these cases, rename can
----------------
maybe just `GetSingleParent(Parent)`, rather than creating a new DynTypeNode.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:390
+      if (const auto *Result =
+              CheckDeclStmt(dyn_cast<DeclStmt>(For->getInit()), NewName))
+        return Result;
----------------
be careful about nullptr here, the init-statement could be a nullptr, which will trigger a crash in dyn_cast, thinking about the case below:

```
for (; ; ;) {
  int var; 
}
```

use `dyn_cast_or_null`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95925