[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

This revision was automatically updated to reflect the committed changes. Closed by commit rG38bdb94120b7: [clangd] Fix rename for explicit destructor calls (authored by kbobyrev). Changed prior to commit: https://reviews.llvm.org/D72638?vs=239182=239227#toc Repository: rG LLVM Github

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev updated this revision to Diff 239182. kbobyrev marked 2 inline comments as done. kbobyrev added a comment. Modify the test to address the comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72638/new/ https://reviews.llvm.org/D72638

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:905 + public: + ~$1^Foo() {} + kadircet wrote: > this actually looks troublesome, since we are not returning the functiondecl >

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:887 + public: + $1^operator int(); + }; could you rather have a conversion operator to a tag (like `struct Bar`), with

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. looks good, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72638/new/ https://reviews.llvm.org/D72638

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev updated this revision to Diff 238791. kbobyrev marked 5 inline comments as done. kbobyrev added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72638/new/ https://reviews.llvm.org/D72638 Files:

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev added inline comments. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:882 "10: targets = {ns}\n"}, + // CXX Constructor, destructor and operators. + {R"cpp( hokein wrote: > could we simplify the newly-added test?

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

hokein added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:620 + if (llvm::dyn_cast(E->getMemberDecl())) +NameLocation = E->getEndLoc(); Refs.push_back(ReferenceLoc{E->getQualifierLoc(), kbobyrev wrote: > kadircet

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev marked 3 inline comments as done. kbobyrev added a comment. The patch should probably be correct now. I've submitted the issue I encountered while trying to add more tests for this patch since it seems to be a separate issue (hopefully, I'm doing everything correctly while setting up

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev updated this revision to Diff 238541. kbobyrev added a comment. Avoid duplicate references by filtering out destructor calls Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72638/new/ https://reviews.llvm.org/D72638 Files:

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev planned changes to this revision. kbobyrev added a comment. Going to investigate few interesting cases of `findExplicitReferences` not working as intended and opting out for a clean approach that prevents duplicates from getting into the reference set. Comment at:

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kadircet added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:620 + if (llvm::dyn_cast(E->getMemberDecl())) +NameLocation = E->getEndLoc(); Refs.push_back(ReferenceLoc{E->getQualifierLoc(), instead of patching the source

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev added a comment. Changed reviewer to @kadircet because Sam would be out until the end of the week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72638/new/ https://reviews.llvm.org/D72638

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

merge_guards_bot added a comment. {icon question-circle color=gray} Unit tests: unknown. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts :

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev added a comment. I'm not sure if leaving both `ReferenceLoc`s pointing to the same location is a sensible solution, but merging them seems quite complicated and probably not really worth the effort. I've considered multiple alternative solutions, as described in

[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

kbobyrev created this revision. kbobyrev added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. kbobyrev edited the summary of this revision. kbobyrev added a comment. I'm not sure if leaving both