[clang-tools-extra] [clangd] Remove potential prefix from enum value names (PR #83412)

Christian Kandeler via cfe-commits cfe-commits at lists.llvm.org
Thu May 2 02:12:07 PDT 2024


https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/83412

>From 349edc160e9c13068c42b35321814296bb713a09 Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler at qt.io>
Date: Thu, 29 Feb 2024 12:26:52 +0100
Subject: [PATCH] [clangd] Remove potential prefix from enum value names

... 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.
---
 .../clangd/refactor/tweaks/ScopifyEnum.cpp    | 61 ++++++++++++-----
 .../unittests/tweaks/ScopifyEnumTests.cpp     | 66 +++++++++++++++++--
 clang-tools-extra/docs/ReleaseNotes.rst       |  3 +
 3 files changed, 108 insertions(+), 22 deletions(-)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp b/clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp
index e36b3249bc7b92..2be2bbc422af75 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,45 @@ llvm::Error ScopifyEnum::addClassKeywordToDeclarations() {
 }
 
 llvm::Error ScopifyEnum::scopifyEnumValues() {
-  std::string PrefixToInsert(D->getName());
-  PrefixToInsert += "::";
-  for (auto E : D->enumerators()) {
-    if (auto Err = scopifyEnumValue(*E, PrefixToInsert))
+  StringRef EnumName(D->getName());
+  bool StripPrefix = true;
+  for (const EnumConstantDecl *E : D->enumerators()) {
+    if (!E->getName().starts_with(EnumName)) {
+      StripPrefix = false;
+      break;
+    }
+  }
+  for (const EnumConstantDecl *E : D->enumerators()) {
+    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) {
+          unsigned Length = EnumName.size();
+          if (Content[Offset + Length] == '_')
+            ++Length;
+          return tooling::Replacement(FilePath, Offset, Length, {});
+        };
+        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 +182,18 @@ llvm::Error ScopifyEnum::scopifyEnumValue(const EnumConstantDecl &CD,
         }
         return false;
       };
-      return IsAlreadyScoped()
-                 ? tooling::Replacement()
-                 : tooling::Replacement(FilePath, Offset, 0, Prefix);
+      if (StripPrefix) {
+        const int ExtraLength =
+            Content[Offset + EnumName.size()] == '_' ? 1 : 0;
+        if (IsAlreadyScoped())
+          return tooling::Replacement(FilePath, Offset,
+                                      EnumName.size() + ExtraLength, {});
+        return tooling::Replacement(FilePath, Offset + EnumName.size(),
+                                    ExtraLength, "::");
+      }
+      return IsAlreadyScoped() ? tooling::Replacement()
+                               : tooling::Replacement(FilePath, Offset, 0,
+                                                      EnumName.str() + "::");
     };
     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..5da059faaf5e8c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ScopifyEnumTests.cpp
@@ -26,7 +26,7 @@ enum ^E;
 )cpp");
 }
 
-TEST_F(ScopifyEnumTest, ApplyTest) {
+TEST_F(ScopifyEnumTest, ApplyTestWithPrefix) {
   std::string Original = R"cpp(
 enum ^E { EV1, EV2, EV3 };
 enum E;
@@ -39,13 +39,69 @@ 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";
+  FileName = "Test.cpp";
+  SCOPED_TRACE(Original);
+  EXPECT_EQ(apply(Original), Expected);
+}
+
+TEST_F(ScopifyEnumTest, ApplyTestWithPrefixAndUnderscore) {
+  std::string Original = R"cpp(
+enum ^E { E_V1, E_V2, E_V3 };
+enum E;
+E func(E in)
+{
+  E out = E_V1;
+  if (in == E_V2)
+    out = E::E_V3;
+  return out;
+}
+)cpp";
+  std::string Expected = R"cpp(
+enum class E { V1, V2, V3 };
+enum class E;
+E func(E in)
+{
+  E out = E::V1;
+  if (in == E::V2)
+    out = E::V3;
+  return out;
+}
+)cpp";
+  FileName = "Test.cpp";
+  SCOPED_TRACE(Original);
+  EXPECT_EQ(apply(Original), Expected);
+}
+
+TEST_F(ScopifyEnumTest, ApplyTestWithoutPrefix) {
+  std::string Original = R"cpp(
+enum ^E { V1, V2, V3 };
+enum E;
+E func(E in)
+{
+  E out = V1;
+  if (in == V2)
+    out = E::V3;
+  return out;
+}
+)cpp";
+  std::string Expected = R"cpp(
+enum class E { V1, V2, V3 };
+enum class E;
+E func(E in)
+{
+  E out = E::V1;
+  if (in == E::V2)
+    out = E::V3;
   return out;
 }
 )cpp";
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d59da3a61b7b61..fb7c45aa759cdd 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -69,6 +69,9 @@ Code completion
 Code actions
 ^^^^^^^^^^^^
 
+- The tweak for turning unscoped into scoped enums now removes redundant prefixes
+  from the enum values.
+
 Signature help
 ^^^^^^^^^^^^^^
 



More information about the cfe-commits mailing list