[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
sam-mccall wrote:
> For some context: When I’m talking about * finding the ranges to rename based
> on an index that’s not clangd’s built-in index* I meant a request like
> [apple#7973](https://github.com/apple/llvm-project/pull/7973).
I see. That makes sense, it's just a bit non-obvious because we don't usually
design these pieces as libraries to be consumed outside clangd (either by code
calling into clangd's internals, or a modified version of clangd).
I think we usually won't support major design changes to accommodate these, but
small ones like this look totally fine.
(There are exceptions, e.g. adding XPC support required significant changes.
After these, XPC lives in llvm-project but could just as easily be downstream).
> Functionality-wise, I would be fine with using a `optional>`
> instead of `Selector` in `collectRenameIdentifierRanges`. FWIW I disagree
> that using a `vector` is cleaner than using a dedicated type for it
> because there’s no type-level information about what the `vector`
> represents
Yeah, "clean" can mean various things, e.g. "clearly communicating intent" here
is in tension with "fewer moving parts".
I'd be fine with `struct SelectorName { vector Chunks; }` or `using
SelectorName = vector`, more than that is (for my taste) more noise
than signal here. I think it should go in `clangd/refactor/Rename.h` though -
it's just a part of that API.
https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
ahoppen wrote: For some context: When I’m talking about * finding the ranges to rename based on an index that’s not clangd’s built-in index* I meant a request like https://github.com/apple/llvm-project/pull/7973. This allows us to use Apple’s IndexStore to find the locations of symbols to rename and then invoke clangd to get the edits to rename the symbols and deal with the parsing of Objective-C selector pieces. Functionality-wise, I would be fine with using a `optional>` instead of `Selector` in `collectRenameIdentifierRanges`. FWIW I disagree that using a `vector` is cleaner than using a dedicated type for it because there’s no type-level information about what the `vector` represents and that `SymbolName` could be made to some day support the cases you mention in https://github.com/llvm/llvm-project/pull/82061#discussion_r1500161008. But I’m new to clangd’s development and will follow your guidance here if you prefer `vector`. https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
@@ -27,19 +31,45 @@ namespace tooling {
/// // ^~ string 0 ~ ^~ string 1 ~
/// \endcode
class SymbolName {
+ llvm::SmallVector NamePieces;
sam-mccall wrote:
I'm a little wary of using this class as:
- it seems like the design has only been informed by objective-C selectors.
It's nice to abstract away difficulties of dealing with names, but this doesn't
handle many (scope qualifiers, operator names, UDL-names) and it's not clear
how/whether it should. If not, I think clangd is better off without the layer
of indirection.
- it's not really clear what you would *do* with this model (name chunks as
strings) other than semi-textual rename. If it's a helper class as part of
tooling/refactor/rename's API, it might be helpful if it were named/documented
as such.
https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
@@ -27,19 +31,45 @@ namespace tooling {
/// // ^~ string 0 ~ ^~ string 1 ~
/// \endcode
class SymbolName {
+ llvm::SmallVector NamePieces;
+
public:
- explicit SymbolName(StringRef Name) {
-// While empty symbol names are valid (Objective-C selectors can have empty
-// name pieces), occurrences Objective-C selectors are created using an
-// array of strings instead of just one string.
-assert(!Name.empty() && "Invalid symbol name!");
-this->Name.push_back(Name.str());
- }
+ SymbolName();
+
+ /// Create a new \c SymbolName with the specified pieces.
+ explicit SymbolName(ArrayRef NamePieces);
+ explicit SymbolName(ArrayRef NamePieces);
+
+ explicit SymbolName(const DeclarationName );
- ArrayRef getNamePieces() const { return Name; }
+ /// Creates a \c SymbolName from the given string representation.
+ ///
+ /// For Objective-C symbol names, this splits a selector into multiple pieces
+ /// on `:`. For all other languages the name is used as the symbol name.
sam-mccall wrote:
This looks too much like guesswork. For example in an ObjC++ file (which is our
default parse language for *.h), SymbolName(`operator std::string`) will be
parsed as the ObjC selector `[" operator std", "", "string"].
If we want to clearly distinguish the different types of names (as clang AST
does) then we should avoid implicit conversions like this where it's easy to
confuse the encoded/decoded state.
If we're content to mostly use strings and heuristically interpret them as
selector names at the edges, that seems OK too - but then this class could just
be a "split" function or so.
(This is up to you - I don't mind much what this API looks like unless we end
up depending on it)
https://github.com/llvm/llvm-project/pull/82061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/sam-mccall requested changes to this pull request. As mentioned: unless there's a clear reason, I'd avoid not to add the dependency on Tooling/Refactoring as it doesn't seem to simplify the implementation much. (My last comment should have been on the review - sorry, I'm still bad at github!) https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/sam-mccall edited https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
sam-mccall wrote: I don't know that this class brings enough value to warrant the dependency - we don't really seem to be simplifying the code, we're mostly just using it as a fancy `vector`. (For some context: we went to some effort in the past to untangle this from tooling/Refactoring/Rename, and a lot of pieces of clangd make tradeoffs between keeping design simple and handling all the special cases of C, C++, ObjC precisely). Is the underlying goal here to be able to use `adjustRenameRanges` from outside of clangd? (That's my reading of "finding the ranges to rename based on an index that’s not clangd’s built-in index" - if you were doing this inside clangd, ISTM you'd have a Selector regardless of the index used). We don't use the selector for anything other than the text chunks it contains, so I think you could just replace `optional` with `optional>` there. (I don't think there are any plans to make use of Selector in other ways, @kadircet @DavidGoldman would know) https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring Rename/USRFinder.cpp Rename/USRFindingAction.cpp Rename/USRLocFinder.cpp + SymbolName.cpp ahoppen wrote: Goode suggestions https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/ahoppen updated
https://github.com/llvm/llvm-project/pull/82061
>From fca2389759d73380312e284c05ddc1662209de2e Mon Sep 17 00:00:00 2001
From: Alex Hoppen
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
---
clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++
clang-tools-extra/clangd/refactor/Rename.h| 6 +-
.../clangd/unittests/RenameTests.cpp | 6 +-
.../Tooling/Refactoring/Rename/SymbolName.h | 50 ++---
clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 +
.../Refactoring/Rename/RenamingAction.cpp | 4 +-
.../Tooling/Refactoring/Rename/SymbolName.cpp | 70 +++
.../Refactoring/Rename/USRLocFinder.cpp | 4 +-
.../Tooling/RefactoringActionRulesTest.cpp| 6 +-
9 files changed, 150 insertions(+), 55 deletions(-)
create mode 100644 clang/lib/Tooling/Refactoring/Rename/SymbolName.cpp
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
@@ -13,6 +13,7 @@ add_clang_library(clangToolingRefactoring Rename/USRFinder.cpp Rename/USRFindingAction.cpp Rename/USRLocFinder.cpp + SymbolName.cpp bnbarham wrote: Should `SymbolName.cpp` live in `Rename` to match its header? https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/bnbarham approved this pull request. https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/bnbarham edited https://github.com/llvm/llvm-project/pull/82061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/ahoppen updated
https://github.com/llvm/llvm-project/pull/82061
>From a8f769d2376e01c789ebf10df95e18b8c23cb79f Mon Sep 17 00:00:00 2001
From: Alex Hoppen
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
---
clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++
clang-tools-extra/clangd/refactor/Rename.h| 6 +-
.../clangd/unittests/RenameTests.cpp | 6 +-
.../Tooling/Refactoring/Rename/SymbolName.h | 50 ++---
clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 +
.../Refactoring/Rename/RenamingAction.cpp | 4 +-
.../Refactoring/Rename/USRLocFinder.cpp | 4 +-
clang/lib/Tooling/Refactoring/SymbolName.cpp | 70 +++
.../Tooling/RefactoringActionRulesTest.cpp| 6 +-
9 files changed, 150 insertions(+), 55 deletions(-)
create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/ahoppen updated
https://github.com/llvm/llvm-project/pull/82061
>From 8b4d8134f581dd44595f347e2ca2465a069411a4 Mon Sep 17 00:00:00 2001
From: Alex Hoppen
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
---
clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++
clang-tools-extra/clangd/refactor/Rename.h| 6 +-
.../clangd/unittests/RenameTests.cpp | 6 +-
.../Tooling/Refactoring/Rename/SymbolName.h | 50 ++---
clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 +
.../Refactoring/Rename/RenamingAction.cpp | 4 +-
.../Refactoring/Rename/USRLocFinder.cpp | 4 +-
clang/lib/Tooling/Refactoring/SymbolName.cpp | 70 +++
.../Tooling/RefactoringActionRulesTest.cpp| 6 +-
9 files changed, 150 insertions(+), 55 deletions(-)
create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/ahoppen updated
https://github.com/llvm/llvm-project/pull/82061
>From be2388c8552ea7a4466046c4d2c9b041a5bf78f2 Mon Sep 17 00:00:00 2001
From: Alex Hoppen
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
---
clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++
clang-tools-extra/clangd/refactor/Rename.h| 6 +-
.../clangd/unittests/RenameTests.cpp | 6 +-
.../Tooling/Refactoring/Rename/SymbolName.h | 50 ++---
clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 +
.../Refactoring/Rename/RenamingAction.cpp | 4 +-
.../Refactoring/Rename/USRLocFinder.cpp | 4 +-
clang/lib/Tooling/Refactoring/SymbolName.cpp | 70 +++
.../Tooling/RefactoringActionRulesTest.cpp| 6 +-
9 files changed, 150 insertions(+), 55 deletions(-)
create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
llvmbot wrote:
@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-clang
Author: Alex Hoppen (ahoppen)
Changes
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
CC @DavidGoldman @bnbarham @kadircet
---
Full diff: https://github.com/llvm/llvm-project/pull/82061.diff
8 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+25-32)
- (modified) clang-tools-extra/clangd/refactor/Rename.h (+3-3)
- (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+3-3)
- (modified) clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
(+40-10)
- (modified) clang/lib/Tooling/Refactoring/CMakeLists.txt (+2)
- (modified) clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp (+2-2)
- (modified) clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (+2-2)
- (added) clang/lib/Tooling/Refactoring/SymbolName.cpp (+70)
``diff
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
unsigned Last = Tokens.size() - 1;
@@ -717,7 +720,7 @@ std::vector collectRenameIdentifierRanges(
// Check if we can find all the relevant
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
llvmbot wrote:
@llvm/pr-subscribers-clang-tools-extra
Author: Alex Hoppen (ahoppen)
Changes
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
CC @DavidGoldman @bnbarham @kadircet
---
Full diff: https://github.com/llvm/llvm-project/pull/82061.diff
8 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/Rename.cpp (+25-32)
- (modified) clang-tools-extra/clangd/refactor/Rename.h (+3-3)
- (modified) clang-tools-extra/clangd/unittests/RenameTests.cpp (+3-3)
- (modified) clang/include/clang/Tooling/Refactoring/Rename/SymbolName.h
(+40-10)
- (modified) clang/lib/Tooling/Refactoring/CMakeLists.txt (+2)
- (modified) clang/lib/Tooling/Refactoring/Rename/RenamingAction.cpp (+2-2)
- (modified) clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (+2-2)
- (added) clang/lib/Tooling/Refactoring/SymbolName.cpp (+70)
``diff
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a method declaration or definition which isn't at the top level or
// similar looking expressions (e.g. an @selector() expression).
llvm::SmallVector Closes;
- llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
+ llvm::StringRef FirstSelPiece = Name.getNamePieces()[0];
auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
unsigned Last = Tokens.size() - 1;
@@ -717,7 +720,7 @@ std::vector collectRenameIdentifierRanges(
// Check if we can find all the relevant selector peices
[clang] [clang-tools-extra] [clangd] Use `SymbolName` to represent Objective-C selectors (PR #82061)
https://github.com/ahoppen created
https://github.com/llvm/llvm-project/pull/82061
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
CC @DavidGoldman @bnbarham @kadircet
>From 7eb837b74f30e55423e1ace6fe29fdec6774f95b Mon Sep 17 00:00:00 2001
From: Alex Hoppen
Date: Fri, 16 Feb 2024 14:50:25 -0800
Subject: [PATCH] [clangd] Use `SymbolName` to represent Objective-C selectors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is a cleaner design than using identifier and an optional `Selector`. It
also allows rename of Objective-C method names if no declaration is at hand and
thus no `Selector` instance can be formed. For example, when finding the ranges
to rename based on an index that’s not clangd’s built-in index.
---
clang-tools-extra/clangd/refactor/Rename.cpp | 57 +++
clang-tools-extra/clangd/refactor/Rename.h| 6 +-
.../clangd/unittests/RenameTests.cpp | 6 +-
.../Tooling/Refactoring/Rename/SymbolName.h | 50 ++---
clang/lib/Tooling/Refactoring/CMakeLists.txt | 2 +
.../Refactoring/Rename/RenamingAction.cpp | 4 +-
.../Refactoring/Rename/USRLocFinder.cpp | 4 +-
clang/lib/Tooling/Refactoring/SymbolName.cpp | 70 +++
8 files changed, 147 insertions(+), 52 deletions(-)
create mode 100644 clang/lib/Tooling/Refactoring/SymbolName.cpp
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp
b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..bd2fcbb7ab1008 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -40,6 +40,8 @@ namespace clang {
namespace clangd {
namespace {
+using tooling::SymbolName;
+
std::optional filePath(const SymbolLocation ,
llvm::StringRef HintFilePath) {
if (!Loc)
@@ -591,11 +593,11 @@ bool isMatchingSelectorName(const syntax::Token ,
const syntax::Token ,
// The search will terminate upon seeing Terminator or a ; at the top level.
std::optional
findAllSelectorPieces(llvm::ArrayRef Tokens,
- const SourceManager , Selector Sel,
+ const SourceManager , const SymbolName ,
tok::TokenKind Terminator) {
assert(!Tokens.empty());
- unsigned NumArgs = Sel.getNumArgs();
+ unsigned NumArgs = Name.getNamePieces().size();
llvm::SmallVector Closes;
std::vector SelectorPieces;
for (unsigned Index = 0, Last = Tokens.size(); Index < Last - 1; ++Index) {
@@ -605,12 +607,12 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ Name.getNamePieces()[PieceCount])) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name. We don't currently properly support empty
// selectors since we may lex them improperly due to ternary statements
// as well as don't properly support storing their ranges for edits.
-if (!Sel.getNameForSlot(PieceCount).empty())
+if (!Name.getNamePieces()[PieceCount].empty())
++Index;
SelectorPieces.push_back(
halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
@@ -662,16 +664,17 @@ findAllSelectorPieces(llvm::ArrayRef
Tokens,
/// Collects all ranges of the given identifier/selector in the source code.
///
-/// If a selector is given, this does a full lex of the given source code in
-/// order to identify all selector fragments (e.g. in method exprs/decls) since
-/// they are non-contiguous.
-std::vector collectRenameIdentifierRanges(
-llvm::StringRef Identifier, llvm::StringRef Content,
-const LangOptions , std::optional Selector) {
+/// If `Name` is an Objective-C symbol name, this does a full lex of the given
+/// source code in order to identify all selector fragments (e.g. in method
+/// exprs/decls) since they are non-contiguous.
+std::vector
+collectRenameIdentifierRanges(const tooling::SymbolName ,
+ llvm::StringRef Content,
+ const LangOptions ) {
std::vector Ranges;
- if (!Selector) {
+ if (auto SinglePiece = Name.getSinglePiece()) {
auto IdentifierRanges =
-collectIdentifierRanges(Identifier, Content, LangOpts);
+collectIdentifierRanges(*SinglePiece, Content, LangOpts);
for (const auto : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -685,7 +688,7 @@ std::vector collectRenameIdentifierRanges(
// parsing a
