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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:263
+  DeclContextLookupResult LookupResult;
+  switch (DC->getDeclKind()) {
+  case Decl::TranslationUnit:
----------------
explain this list somewhat?
e.g. these are the declcontexts which form a namespace where conflicts can occur?
and why function doesn't belong here


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:263
+  DeclContextLookupResult LookupResult;
+  switch (DC->getDeclKind()) {
+  case Decl::TranslationUnit:
----------------
sammccall wrote:
> explain this list somewhat?
> e.g. these are the declcontexts which form a namespace where conflicts can occur?
> and why function doesn't belong here
I think you want to walk up DC while isTransparentContext()


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:315
+  // conflicts if we perform the rename.
+  // (!) DeclContext::lookup doesn't perform lookup local decls in a
+  // function-kind DeclContext.
----------------
Can you explain this a bit more (e.g. why?)

My guess is given:

```
void foo() {
  int bar;
}
```

`foo` is a DC but `bar` is not declared directly within it (rather within the CompoundStmt that is its body), therefore will not be looked up.

In which case an explanation like "Note that the enclosing DeclContext may not be the enclosing scope, particularly for function-local declarations, so this has both false positives and negatives." might help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89790