[PATCH] D88383: [clangd] Add a tweak for filling in enumerators of a switch statement.

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo created this revision.
tdeo added a reviewer: sammccall.
tdeo added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, mgorny.
Herald added a project: clang.
tdeo requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Add a tweak that populates an empty switch statement of an enumeration type 
with all of the enumerators of that type.

Before:

  enum Color { RED, GREEN, BLUE };
  void f(Color color) {
switch (color) {}
  }

After:

  enum Color { RED, GREEN, BLUE };
  void f(Color color) {
switch (color) {
case RED:
case GREEN:
case BLUE:
  break;
}
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88383

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2802,6 +2802,96 @@
   }
 }
 
+TWEAK_TEST(PopulateSwitch);
+TEST_F(PopulateSwitchTest, Test) {
+  struct Case {
+CodeContext Context;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  };
+
+  Case Cases[]{
+  {
+  // No enumerators
+  Function,
+  R""(enum Enum {}; ^switch ((Enum)0) {})"",
+  "unavailable",
+  },
+  {
+  // Existing enumerators in switch
+  Function,
+  R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+  "unavailable",
+  },
+  {
+  // Body not CompoundStmt
+  Function,
+  R""(enum Enum {A}; ^switch (A);)"",
+  "unavailable",
+  },
+  {
+  // Selection on switch token
+  Function,
+  R""(enum Enum {A}; ^switch (A) {})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Selection on switch condition
+  Function,
+  R""(enum Enum {A}; switch (^A) {})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Selection in switch body
+  Function,
+  R""(enum Enum {A}; switch (A) {^})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Scoped enumeration
+  Function,
+  R""(enum class Enum {A}; ^switch (Enum::A) {})"",
+  R""(enum class Enum {A}; switch (Enum::A) {case Enum::A:break;})"",
+  },
+  {
+  // Scoped enumeration with multiple enumerators
+  Function,
+  R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
+  R""(enum class Enum {A,B}; )""
+  R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+  },
+  {
+  // Scoped enumerations in namespace
+  File,
+  R""(
+namespace ns { enum class Enum {A}; }
+void function() { ^switch (ns::Enum::A) {} }
+  )"",
+  R""(
+namespace ns { enum class Enum {A}; }
+void function() { switch (ns::Enum::A) {case ns::Enum::A:break;} }
+  )"",
+  },
+  {
+  // Unscoped enumerations in namespace
+  File,
+  R""(
+namespace ns { enum Enum {A}; }
+void function() { ^switch (ns::A) {} }
+  )"",
+  R""(
+namespace ns { enum Enum {A}; }
+void function() { switch (ns::A) {case ns::A:break;} }
+  )"",
+  },
+  };
+
+  for (const auto &Case : Cases) {
+Context = Case.Context;
+EXPECT_EQ(apply(Case.TestSource), Case.ExpectedSource);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -0,0 +1,145 @@
+//===--- PopulateSwitch.cpp --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tweak that populates an empty switch statement of an enumeration type with
+// all of the enumerators of that type.
+//
+// Before:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+// switch (color) {}
+//   }
+//
+// After:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+// switch (color) {
+// case RED:
+// case GREEN:
+// case BLUE:
+//   break;
+// }
+//   }
+//
+//===

[PATCH] D88383: [clangd] Add a tweak for filling in enumerators of a switch statement.

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo updated this revision to Diff 294644.
tdeo added a comment.

- Check body is empty instead of having no case statements.
- Null check the return value from SwitchStmt::getCond.
- Insert replacement before the right bracket instead of the left one, for 
future improvements.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88383/new/

https://reviews.llvm.org/D88383

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2802,6 +2802,96 @@
   }
 }
 
+TWEAK_TEST(PopulateSwitch);
+TEST_F(PopulateSwitchTest, Test) {
+  struct Case {
+CodeContext Context;
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  };
+
+  Case Cases[]{
+  {
+  // No enumerators
+  Function,
+  R""(enum Enum {}; ^switch ((Enum)0) {})"",
+  "unavailable",
+  },
+  {
+  // Existing enumerators in switch
+  Function,
+  R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+  "unavailable",
+  },
+  {
+  // Body not CompoundStmt
+  Function,
+  R""(enum Enum {A}; ^switch (A);)"",
+  "unavailable",
+  },
+  {
+  // Selection on switch token
+  Function,
+  R""(enum Enum {A}; ^switch (A) {})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Selection on switch condition
+  Function,
+  R""(enum Enum {A}; switch (^A) {})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Selection in switch body
+  Function,
+  R""(enum Enum {A}; switch (A) {^})"",
+  R""(enum Enum {A}; switch (A) {case A:break;})"",
+  },
+  {
+  // Scoped enumeration
+  Function,
+  R""(enum class Enum {A}; ^switch (Enum::A) {})"",
+  R""(enum class Enum {A}; switch (Enum::A) {case Enum::A:break;})"",
+  },
+  {
+  // Scoped enumeration with multiple enumerators
+  Function,
+  R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
+  R""(enum class Enum {A,B}; )""
+  R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+  },
+  {
+  // Scoped enumerations in namespace
+  File,
+  R""(
+namespace ns { enum class Enum {A}; }
+void function() { ^switch (ns::Enum::A) {} }
+  )"",
+  R""(
+namespace ns { enum class Enum {A}; }
+void function() { switch (ns::Enum::A) {case ns::Enum::A:break;} }
+  )"",
+  },
+  {
+  // Unscoped enumerations in namespace
+  File,
+  R""(
+namespace ns { enum Enum {A}; }
+void function() { ^switch (ns::A) {} }
+  )"",
+  R""(
+namespace ns { enum Enum {A}; }
+void function() { switch (ns::A) {case ns::A:break;} }
+  )"",
+  },
+  };
+
+  for (const auto &Case : Cases) {
+Context = Case.Context;
+EXPECT_EQ(apply(Case.TestSource), Case.ExpectedSource);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -0,0 +1,146 @@
+//===--- PopulateSwitch.cpp --*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tweak that populates an empty switch statement of an enumeration type with
+// all of the enumerators of that type.
+//
+// Before:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+// switch (color) {}
+//   }
+//
+// After:
+//   enum Color { RED, GREEN, BLUE };
+//
+//   void f(Color color) {
+// switch (color) {
+// case RED:
+// case GREEN:
+// case BLUE:
+//   break;
+// }
+//   }
+//
+//===--===//
+
+#include "AST.h"
+#include "Selection.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include 
+
+namespace clang {
+namespace clangd {
+n

[PATCH] D88383: [clangd] Add a tweak for filling in enumerators of a switch statement.

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo added a comment.

@sammccall, I don't have commit access, so if this revision has no issues, can 
you land it? Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88383/new/

https://reviews.llvm.org/D88383

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88383: [clangd] Add a tweak for filling in enumerators of a switch statement.

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo added a comment.

In D88383#2297778 , @sammccall wrote:

> In D88383#2297735 , @tdeo wrote:
>
>> @sammccall, I don't have commit access, so if this revision has no issues, 
>> can you land it? Thanks.
>
> Absolutely, thanks a lot for the patch!
>
> Any interest in extending to non-empty switches? Or better for someone else 
> to pick that up?

I'd like to give it a shot soon, but I wouldn't mind if someone else did it 
first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88383/new/

https://reviews.llvm.org/D88383

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo created this revision.
tdeo added a reviewer: sammccall.
tdeo added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
tdeo requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Improve the recently-added PopulateSwitch tweak to work on non-empty switches.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88434

Files:
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,33 @@
   "unavailable",
   },
   {
-  // Existing enumerators in switch
+  // All enumerators already in switch (single, unscoped)
   Function,
-  R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+  R""(enum Enum {A}; ^switch (A) {case A:break;})"",
+  "unavailable",
+  },
+  {
+  // All enumerators already in switch (multiple, unscoped)
+  Function,
+  R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+  "unavailable",
+  },
+  {
+  // All enumerators already in switch (single, scoped)
+  Function,
+  R""(
+enum class Enum {A};
+^switch (Enum::A) {case Enum::A:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // All enumerators already in switch (multiple, scoped)
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+  )"",
   "unavailable",
   },
   {
@@ -2867,9 +2891,53 @@
   {
   // Scoped enumeration with multiple enumerators
   Function,
-  R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
-  R""(enum class Enum {A,B}; )""
-  R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+  )"",
+  R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (unscoped)
+  Function,
+  R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+  )"",
+  R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators,
+  // even when using integer literals
+  Function,
+  R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+  )"",
+  R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (scoped)
+  Function,
+  R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+  )"",
+  R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+  )"",
   },
   {
   // Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,14 @@
 #include "AST.h"
 #include "Selection.h"
 #include "refactor/Tweak.h"
+#include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
 #include 
 
 namespace clang {
@@ -52,18 +54,16 @@
   Intent intent() const override { return Refactor; }
 
 private:
-  ASTContext *ASTCtx = nullptr;
   const DeclContext *DeclCtx = nullptr;
   const SwitchStmt *Switch = nullptr;
   const CompoundStmt *Body = nullptr;
+  const EnumType *EnumT = nullptr;
   const EnumDecl *EnumD = nullptr;
 };
 
 REGISTER_TWEAK(PopulateSwitch)
 
 bool PopulateSwitch::prepare(const Selection &Sel) {
-  ASTCtx = &Sel.AST->getASTContext();
-
   const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
   if (!CA)
 return false;
@@ -94,11 +94,6 @@
   if (!Body)
 return false;
 
-  // Since we currently always insert all enumerators, don't suggest this tweak
-  // if the body is not empty.
-  if (!Body->body_

[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo updated this revision to Diff 294959.
tdeo added a comment.

- Improve comments
- In apply(), assert on the conditions we established in prepare() instead of 
proceeding.
- Remove redundant tests and add new ones for value-dependent and GNU range 
cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88434/new/

https://reviews.llvm.org/D88434

Files:
  clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,48 @@
   "unavailable",
   },
   {
-  // Existing enumerators in switch
+  // All enumerators already in switch (unscoped)
   Function,
-  R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+  R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+  "unavailable",
+  },
+  {
+  // All enumerators already in switch (scoped)
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // Default case in switch
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {default:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // GNU range in switch
+  Function,
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A ... Enum::B:break;}
+  )"",
+  "unavailable",
+  },
+  {
+  // Value dependent case expression
+  File,
+  R""(
+enum class Enum {A,B};
+template
+void function() {
+^switch (Enum::A) {case Value:break;}
+}
+  )"",
   "unavailable",
   },
   {
@@ -2867,9 +2906,53 @@
   {
   // Scoped enumeration with multiple enumerators
   Function,
-  R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
-  R""(enum class Enum {A,B}; )""
-  R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+  R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+  )"",
+  R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (unscoped)
+  Function,
+  R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+  )"",
+  R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators,
+  // even when using integer literals
+  Function,
+  R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+  )"",
+  R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+  )"",
+  },
+  {
+  // Only filling in missing enumerators (scoped)
+  Function,
+  R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+  )"",
+  R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+  )"",
   },
   {
   // Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,15 @@
 #include "AST.h"
 #include "Selection.h"
 #include "refactor/Tweak.h"
+#include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
+#include 
 #include 
 
 namespace clang {
@@ -52,18 +55,16 @@
   Intent intent() const override { return Refactor; }
 
 private:
-  ASTContext *ASTCtx = nullptr;
   const DeclContext *DeclCtx = nullptr;
   const SwitchStmt *Switch = nullptr;
   const CompoundStmt *Body = nullptr;
+  const EnumType *EnumT = nullptr;
   const EnumDecl *EnumD = nullptr;
 };
 
 REGISTER_TWEAK(PopulateSwitch)
 
 bool PopulateSwitch::prepare(const Selection &Sel) {
-  ASTCtx = &Sel.AST->getASTContext();
-
   const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
   if (

[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches

[Tadeo Kondrak via Phabricator via cfe-commits]

tdeo marked 10 inline comments as done.
tdeo added a comment.

Thanks for the review! Please land this if there are no more issues.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88434/new/

https://reviews.llvm.org/D88434

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits