hokein added a comment.
In D69263#1718525 <https://reviews.llvm.org/D69263#1718525>, @ilya-biryukov
wrote:
> Not sure that holds. What if the file in question is being rebuilt right now?
> We do not wait until all ASTs are built (and the dynamic index gets the new
> results).
> Open files actually pose an interesting problem: their contents do not
> correspond to the file contents on disk.
> We should choose which of those we update on rename: dirty buffers or file
> contents? (I believe we should do dirty buffers, but I believe @sammccall has
> the opposite opinion, so we should sync on this)
Based on the offline discussion, we decide to use dirty buffers for opened
files.
================
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
----------------
ilya-biryukov wrote:
> 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.
Done. I tried my best to unify them, please take a look on the new code.
================
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();
----------------
ilya-biryukov wrote:
> 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?
I prefer to do it afterwards as the patch is relatively big now.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380
+ auto MainFileRenameEdit =
+ renameWithinFile(AST, RenameDecl, RInputs.NewName);
+ if (!MainFileRenameEdit)
----------------
ilya-biryukov wrote:
> 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?
yes, exactly. I have simplified the code in this function, and added
documentations.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:357
+ // Handle within-file rename.
+ auto Reject = renamableWithinFile(*RenameDecl->getCanonicalDecl(),
+ RInputs.MainFilePath, RInputs.Index);
----------------
ilya-biryukov wrote:
> 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?
I was attempting to keep two branches (within-file rename, cross-file rename)
as separately as possible, but I agree with your suggestion, it does simplify
the code.
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
