hokein added a comment. In D69263#1718525 <https://reviews.llvm.org/D69263#1718525>, @ilya-biryukov wrote:
> In D69263#1717985 <https://reviews.llvm.org/D69263#1717985>, @hokein wrote: > > > Thinking more about this -- we have a dynamic index (for all opened files) > > which is overlaid on a static index (which is a background index in > > open-source world), so for all affected files, they are either in > > > > 1. an open state -- we can rely on the dynamic index, I think it is safe to > > assume that index always returns up-to-date results; > > > 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). I'm not sure this would happen quite often in practice (probably depends on users' behavior). but yes, patching ranges would mitigate this issue. > 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) I have the same feeling of using dirty buffers for open files, let's discuss offline. >> 2. a non-open state -- rely on the background index, however background >> index has more chance to be stale (especially we don't detect file-change >> events at the moment), we could do a range patch heuristically to mitigate >> this stale issue. Failing that, we surface the error to users. > > To summarize, I think files can be stale in both cases and we should patch > the ranges in both cases. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:268 - // decide to implement renaming with index support. - if ((Roles & static_cast<unsigned>(index::SymbolRole::NameReference))) - return true; ---------------- ilya-biryukov wrote: > Not sure what was filtered out here. > I suggest moving this to a separate change, with unit-tests clearly > describing the new symbols we do not filter out anymore and an explanation > why it's necessary. > > WDYT? Agreed. split it out. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:119 +llvm::Optional<ReasonToReject> +renameableOutsideFile(const NamedDecl &RenameDecl, const SymbolIndex *Index) { ---------------- 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); ``` ================ 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: > 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. ================ 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: > 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. ================ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:380 + auto MainFileRenameEdit = + renameWithinFile(AST, RenameDecl, RInputs.NewName); + if (!MainFileRenameEdit) ---------------- 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... ================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:32 + + bool AllowCrossFile = false; // true if enable cross-file rename +}; ---------------- ilya-biryukov wrote: > ilya-biryukov wrote: > > Have we considered post-filtering instead? > > Or are we concerned with performance implications of openning too many > > files to compute the corrected edit ranges? > > > > Probably worth documenting why we need it. > NIT: the comment is redundant Performance is one of the concern. I think the main purpose of this flag is to avoid any regressions in our existing within-file rename while developing the new cross-file rename. 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
