[PATCH] D89790: [clangd] Add basic conflict detection for the rename.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! Just style nits.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:266
+  switch (DC->getDeclKind()) {
+  // The enclosing DeclContext may not be the enclosing scope, it might have
+  // false positives and negatives, so we only choose the DeclContexts that
----------------
this comment is missing the *reason*.

FunctionDecl is a good example, but the fundamental reason is again not given.
Maybe change that paragraph to:

```
Notably, FunctionDecl is excluded, because local variables are not scoped to the function, but rather to the CompoundStmt that is its body.
Lookup will not find function-local variables.
```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:268
+  // false positives and negatives, so we only choose the DeclContexts that
+  // we have confidence.
+  // (!) FunctionDecl is excluded, becase it is not the enclosing scope for
----------------
confidence in what?

I think the condition is: don't have any subscopes that are neither DeclContexts nor transparent.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:320
+    return llvm::None;
+  // Perform a lookup in the decl context of the RenameDecl, to find out any
+  // conflicts if we perform the rename.
----------------
This is still a bit confusing: *here* you seem to decide which scope to look up in, but then inside the lookup() function you may change your mind.
I'd suggest moving the `getDeclContext()` inside the function and renaming the function to "findSiblingWithName" or something.

Also while early-exit is often good, I find early-exit in the *success* case confusing.
Consider:

```
// Name conflict detection.
// Function conflicts are subtle (overloading), so ignore them.
if (RenameDecl.getKind() != Decl::Function) {
  if (lookupSiblingWithName(ASTCtx, RenameDecl, NewName))
    return InvalidName{...};
}
return llvm::None;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89790