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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
