hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/Rename.h:61 + Range R; + FileEdits Edits; +}; ---------------- sammccall wrote: > hokein wrote: > > sammccall wrote: > > > It's slightly awkward to have the half-populated state (may or may not > > > contain cross-file results, can't tell without the query). > > > > > > I'd consider redundantly including `Edit LocalChanges` and `FileEdits > > > GlobalChanges` with the former always set and the latter empty when > > > returned from preparerename. > > > It's slightly awkward to have the half-populated state (may or may not > > > contain cross-file results, can't tell without the query). > > > > I feel this is not too bad, the query is quite trivial, just `Edits.size() > > > 1`. > > > > > I'd consider redundantly including Edit LocalChanges and FileEdits > > > GlobalChanges with the former always set and the latter empty when > > > returned from preparerename. > > > > This change seems nice to get changes for main-file, but I think > > `GlobalChanges` should include LocalChanges (otherwise, client side needs > > to combine these two when applying the final rename edits)? then we can't > > leave the later empty while keeping the former set. > > > > Happy to do the change, but it looks like we don't have usage of > > `LocalChanges` so far. In prepareRename, we want main-file occurrences, > > `rename` will always return them regardless of single-file or cross-file > > rename, so I think `Edits` is efficient. > > > It's slightly awkward to have the half-populated state (may or may not > > > contain cross-file results, can't tell without the query). > > > > I feel this is not too bad, the query is quite trivial, just `Edits.size() > > > 1`. > This isn't sufficient though: if Edits.size() == 1 then the results may be > either complete or incomplete. (Sorry my wording above may have been poor, > but this is the distinction I was referring to). > > > > I'd consider redundantly including Edit LocalChanges and FileEdits > > > GlobalChanges with the former always set and the latter empty when > > > returned from preparerename. > > > > This change seems nice to get changes for main-file, but I think > > `GlobalChanges` should include LocalChanges (otherwise, client side needs > > to combine these two when applying the final rename edits)? then we can't > > leave the later empty while keeping the former set. > > Sure we can. `// If the full set of changes is unknown, this field is empty.` > > > Happy to do the change, but it looks like we don't have usage of > > `LocalChanges` so far. In prepareRename, we want main-file occurrences, > > `rename` will always return them regardless of single-file or cross-file > > rename, so I think `Edits` is efficient. > > Yes, it's possible to implement correct behavior on top of the current API. > It effectively reuses the same field with different semantics/contracts, and > the client has enough information to know which contract is in place (and > throw away the key in the expected singleton map). > However I think this is fragile and harder to understand than providing > separate fields for the two contracts - using types to distinguish local vs > global results makes the data harder to misuse. Given we expect this to be > used by embedders, I think it's worth adding the second field. ah, I see your points, agreed. Added the LocalChanges. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88634/new/ https://reviews.llvm.org/D88634 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
