-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clangd] Remove potential prefix from enum value names #83412
Conversation
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Christian Kandeler (ckandeler) Changes... when converting unscoped to scoped enums. Full diff: https://github.com/llvm/llvm-project/pull/83412.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp b/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp
index e36b3249bc7b92..5c042ee7b782b9 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp
@@ -40,15 +40,12 @@ namespace {
/// void f() { E e1 = EV1; }
///
/// After:
-/// enum class E { EV1, EV2 };
-/// void f() { E e1 = E::EV1; }
+/// enum class E { V1, V2 };
+/// void f() { E e1 = E::V1; }
///
/// Note that the respective project code might not compile anymore
/// if it made use of the now-gone implicit conversion to int.
/// This is out of scope for this tweak.
-///
-/// TODO: In the above example, we could detect that the values
-/// start with the enum name, and remove that prefix.
class ScopifyEnum : public Tweak {
const char *id() const final;
@@ -63,7 +60,8 @@ class ScopifyEnum : public Tweak {
std::function<tooling::Replacement(StringRef, StringRef, unsigned)>;
llvm::Error addClassKeywordToDeclarations();
llvm::Error scopifyEnumValues();
- llvm::Error scopifyEnumValue(const EnumConstantDecl &CD, StringRef Prefix);
+ llvm::Error scopifyEnumValue(const EnumConstantDecl &CD, StringRef EnumName,
+ bool StripPrefix);
llvm::Expected<StringRef> getContentForFile(StringRef FilePath);
unsigned getOffsetFromPosition(const Position &Pos, StringRef Content) const;
llvm::Error addReplacementForReference(const ReferencesResult::Reference &Ref,
@@ -125,25 +123,42 @@ llvm::Error ScopifyEnum::addClassKeywordToDeclarations() {
}
llvm::Error ScopifyEnum::scopifyEnumValues() {
- std::string PrefixToInsert(D->getName());
- PrefixToInsert += "::";
+ StringRef EnumName(D->getName());
+ bool StripPrefix = true;
+ for (auto E : D->enumerators()) {
+ if (!E->getName().starts_with(EnumName)) {
+ StripPrefix = false;
+ break;
+ }
+ }
for (auto E : D->enumerators()) {
- if (auto Err = scopifyEnumValue(*E, PrefixToInsert))
+ if (auto Err = scopifyEnumValue(*E, EnumName, StripPrefix))
return Err;
}
return llvm::Error::success();
}
llvm::Error ScopifyEnum::scopifyEnumValue(const EnumConstantDecl &CD,
- StringRef Prefix) {
+ StringRef EnumName,
+ bool StripPrefix) {
for (const auto &Ref :
findReferences(*S->AST, getPosition(CD), 0, S->Index, false)
.References) {
- if (Ref.Attributes & ReferencesResult::Declaration)
+ if (Ref.Attributes & ReferencesResult::Declaration) {
+ if (StripPrefix) {
+ const auto MakeReplacement = [&EnumName](StringRef FilePath,
+ StringRef /* Content */,
+ unsigned Offset) {
+ return tooling::Replacement(FilePath, Offset, EnumName.size(), {});
+ };
+ if (auto Err = addReplacementForReference(Ref, MakeReplacement))
+ return Err;
+ }
continue;
+ }
- const auto MakeReplacement = [&Prefix](StringRef FilePath,
- StringRef Content, unsigned Offset) {
+ const auto MakeReplacement = [&](StringRef FilePath, StringRef Content,
+ unsigned Offset) {
const auto IsAlreadyScoped = [Content, Offset] {
if (Offset < 2)
return false;
@@ -164,9 +179,15 @@ llvm::Error ScopifyEnum::scopifyEnumValue(const EnumConstantDecl &CD,
}
return false;
};
+ if (StripPrefix) {
+ if (IsAlreadyScoped())
+ return tooling::Replacement(FilePath, Offset, EnumName.size(), {});
+ return tooling::Replacement(FilePath, Offset + EnumName.size(), 0,
+ "::");
+ }
return IsAlreadyScoped()
? tooling::Replacement()
- : tooling::Replacement(FilePath, Offset, 0, Prefix);
+ : tooling::Replacement(FilePath, Offset, 0, EnumName);
};
if (auto Err = addReplacementForReference(Ref, MakeReplacement))
return Err;
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp
index b5a964a5a26d80..45266fdd142cd1 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp
@@ -39,13 +39,13 @@ E func(E in)
}
)cpp";
std::string Expected = R"cpp(
-enum class E { EV1, EV2, EV3 };
+enum class E { V1, V2, V3 };
enum class E;
E func(E in)
{
- E out = E::EV1;
- if (in == E::EV2)
- out = E::EV3;
+ E out = E::V1;
+ if (in == E::V2)
+ out = E::V3;
return out;
}
)cpp";
|
c687b73
to
01f74dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change could use a release note in the docs, could you please add one?
01f74dd
to
78393d2
Compare
Incorporated review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the enumerators start with the enum name, but the names contain a _
as a separator, then applying this tweak will result in _
-prefixed enumerators. Can you please handle that case as well and remove the _
? Otherwise, this looks good to me
... when converting unscoped to scoped enums. With traditional enums, a popular technique to guard against potential name clashes is to use the enum name as a pseudo-namespace, like this: enum MyEnum { MyEnumValue1, MyEnumValue2 }; With scoped enums, this makes no sense, making it extremely unlikely that the user wants to keep such a prefix when modernizing. Therefore, our tweak now removes it.
78393d2
to
349edc1
Compare
Underscore separators get removed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
... when converting unscoped to scoped enums.
With traditional enums, a popular technique to guard against potential name clashes is to use the enum name as a pseudo-namespace, like this:
enum MyEnum { MyEnumValue1, MyEnumValue2 };
With scoped enums, this makes no sense, making it extremely unlikely that the user wants to keep such a prefix when modernizing. Therefore, our tweak now removes it.