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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:263
+  DeclContextLookupResult LookupResult;
+  switch (DC->getDeclKind()) {
+  case Decl::TranslationUnit:
----------------
sammccall wrote:
> 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()
yeah, that's a good point. added.


================
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.
----------------
sammccall wrote:
> 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.
Done. This comment is about why we exclude functionDecl, moved it to the lookup function there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89790