ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119
+llvm::Optional<ReasonToReject>
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
----------------
hokein wrote:
> ilya-biryukov wrote:
> > So `llvm::None` means we do not reject?
> > Probably worth documenting that.
> >
> > Maybe even add an enumerator `ReasonToReject::Accept`, indicating the
> > symbol should be accepted and get rid of `Optional` altogether.
> >
> > Otherwise this leads to code like:
> > ```
> > auto R = renameableOutsideFile(N, I);
> > if (!R) { // negative condition.
> > return doRename(N, I); // But we're running the rename anyway... WAT?
> > }
> > ``
> yeah, returning `None` means that we accept the rename.
>
> Adding `Accept` is not aligning with the mental model, `Accept` doesn't
> belong to `ReasonToReject` I think. I agree the above given code is
> misleading, but looking at the current code in this file, it doesn't seem too
> bad, I think?
>
> ```
> if (auto Reject = renameableOutsideFile(*RenameDecl, RInputs.Index))
> return makeError(*Reject);
> ```
Agreed this aligns with the other functions we have in the file, so let's keep
as is.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:121
+renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) {
+ if (RenameDecl.getParentFunctionOrMethod())
+ return None; // function-local symbols
----------------
hokein wrote:
> ilya-biryukov wrote:
> > This function resembles `renamableWithinFile`.
> > We should probably share implementation and pass an extra flag. Something
> > like `isRenamable(..., bool IsCrossFile)`.
> >
> > WDYT?
> though they look similar, the logic is quite different, for example, we
> query the index in within-file rename to detect a symbol is used outside of
> the file which is not needed for cross-file rename, and cross-file rename
> allows fewer symbols (as our index doesn't support them), so I don't think we
> can share a lot of implementation.
Can this be handled with a special flag:
```
bool renameable(NamedDecl &Decl, const SymbolIndex *Index, bool CrossFile) {
if (CrossFileRename) {
// check something special...
}
}
```
Sharing implementation in the same function makes the differences between
cross-file and single-file case obvious and also ensures any things that need
to be updated for both are shared.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:317
+ std::string OldName = RenameDecl->getNameAsString();
+ for (const auto &FileAndOccurrences : AffectedFiles) {
+ llvm::StringRef FilePath = FileAndOccurrences.first();
----------------
hokein wrote:
> ilya-biryukov wrote:
> > I feel this code is fundamental to the trade-offs we made for rename.
> > Could we move this to a separate function and add unit-tests for it?
> >
> > You probably want to have something that handles just a single file rather
> > than all edits in all files, to avoid mocking VFS in tests, etc.
> >
> >
> Agree, especially when we start implementing complicated stuff, e.g. range
> patching heuristics.
>
> Moved it out, but note that I don't plan to add more stuff in this initial
> patch, so the logic is pretty straightforward, just assembling rename
> replacement from occurrences.
>
> but note that I don't plan to add more stuff in this initial patch
Should we also check whether the replaced text matches the expected identifier
and add unit-tests for this?
Or would you prefer to move all changes to this function to a separate patch?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380
+ auto MainFileRenameEdit =
+ renameWithinFile(AST, RenameDecl, RInputs.NewName);
+ if (!MainFileRenameEdit)
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Can we rely on the fact that dynamic index should not be stale for the
> > current file instead?
> > Or don't we have this invariant?
> >
> > We end up having two implementations now: index-based and AST-based. It'd
> > be nice to have just one.
> > If that's not possible in the first revision, could you explain why?
> >
> > Note that moving the current-file rename to index-based implementation is
> > independent of doing cross-file renames. Maybe we should start with it?
> I think we can't get rid of the AST-based rename -- mainly for renaming local
> symbols (e.g. local variable in function body), as we don't index these
> symbols...
Thanks, I believe you're right... We can't unify these implementations, at
least not in the short term.
So `renameOutsideFile` fails on local symbols and `renameWithinFile` should
handle that, right?
Also worth noting that `renameWithinFile` is AST-based and `renameOutsideFile`
is index-based.
Could we document that?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:357
+ // Handle within-file rename.
+ auto Reject = renamableWithinFile(*RenameDecl->getCanonicalDecl(),
+ RInputs.MainFilePath, RInputs.Index);
----------------
Can we avoid duplicating calls to `renamebleWithinFile`? It should be possible
to only call `renameOutsideFile` when `CrossFileRename` is enabled and always
call `renameWithinFile`. Or is there something I'm missing?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69263/new/
https://reviews.llvm.org/D69263
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
