clangd: Add a break for every case in the PopulateSwitch tweak

[ckandeler]

Summary

Fall-through is not the common case.

[ckandeler]

Fixed unintentional whitespace change.

[njames93]

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp

208–209

[ckandeler]

Improved code as per review comment.

[sammccall]

FWIW I prefer the existing behaviour (reasons below). @kadircet, thoughts?

• The output with break is harder to visually scan. (In styles where break goes on its own line)
• The output is annoying to edit when you *do* want to deal with blocks of cases at once. When editing cases individually anyway, I don't find typing break a burden.
• The goal isn't to provide a correct body, but to avoid having to remember/type all the cases. Any extra output is a distraction.

[kadircet]

I agree with Sam on this one and remember having some (possibly offline) discussions around not having those breaks explicitly (we should've documented the reasoning, sorry).

As pointed out, I believe the main benefit is not spelling out each enum value (it's usually hard to figure out which ones are missing, requires constant switching between enum declaration and switch body, and then do some copy/paste or write them).
After those are spelled the developer needs to make a decision about how they'll be handled anyway and if some enum value requires special handling adding the extra break by hand is not too much of a burden, but deleting those when the intent is to have fall through, is extra burden and it might be unclear which ones are new labels.

[ckandeler]

Fair enough.

[njames93]

The only way I'd suggest supporting this is via an option in config(like we do with the add using tweak), but I don't think even then it would get much use.

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp

 //   }
 //
 // After:
 //   enum Color { RED, GREEN, BLUE };
 //
 //   void f(Color color) {
 //     switch (color) {
 //     case RED:
+//       break;
 //     case GREEN:
+//       break;
 //     case BLUE:
 //       break;
 //     }
 //   }
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
Expected<Tweak::Effect> PopulateSwitch::apply(const Selection &Sel) {
   for (auto &EnumConstant : ExpectedCases) {
     // Skip any enum constants already covered
     if (EnumConstant.second.isCovered())
       continue;
 
     Text.append({"case ", getQualification(DeclASTCtx, DeclCtx, Loc, EnumD)});
     if (EnumD->isScoped())
       Text.append({EnumD->getName(), "::"});
-    Text.append({EnumConstant.second.getEnumConstant()->getName(), ":"});
+    Text.append({EnumConstant.second.getEnumConstant()->getName(), ":break;"});
   }
 
   assert(!Text.empty() && "No enumerators to insert!");
-  Text += "break;";
 
   const SourceManager &SM = Ctx.getSourceManager();
   return Effect::mainFileEdit(
       SM, tooling::Replacements(tooling::Replacement(SM, Loc, 0, Text)));
 }
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/tweaks/PopulateSwitchTests.cpp

Case Cases[]{
           // Scoped enumeration with multiple enumerators
           Function,
           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;}
+            switch (Enum::A) {case Enum::A:break;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;}
+            switch (A) {case B:break;case A:break;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;}
+            switch (A) {case 1:break;case A:break;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;}
+            {case Enum::B:break;case Enum::A:break;case Enum::C:break;}
           )"",
       },
       {
           // Scoped enumerations in namespace
           File,
           R""(
             namespace ns { enum class Enum {A}; }
             void function() { ^switch (ns::Enum::A) {} }
Case Cases[]{
             namespace ns { enum Enum {A}; }
             void function() { switch (ns::A) {case ns::A:break;} }
           )"",
       },
       {
           // Duplicated constant names
           Function,
           R""(enum Enum {A,B,b=B}; ^switch (A) {})"",
-          R""(enum Enum {A,B,b=B}; switch (A) {case A:case B:break;})"",
+          R""(enum Enum {A,B,b=B}; switch (A) {case A:break;case B:break;})"",
       },
       {
           // Duplicated constant names all in switch
           Function,
-          R""(enum Enum {A,B,b=B}; ^switch (A) {case A:case B:break;})"",
+          R""(enum Enum {A,B,b=B}; ^switch (A) {case A:break;case B:break;})"",
           "unavailable",
       },
       {
           // Enum is dependent type
           File,
           R""(template<typename T> void f() {enum Enum {A}; ^switch (A) {}})"",
           "unavailable",
       },
       {// C: Only filling in missing enumerators
        Function,
        R""(
             enum CEnum {A,B,C};
             enum CEnum val = A;
             ^switch (val) {case B:break;}
           )"",
        R""(
             enum CEnum {A,B,C};
             enum CEnum val = A;
-            switch (val) {case B:break;case A:case C:break;}
+            switch (val) {case B:break;case A:break;case C:break;}
           )"",
        "TestTU.c"},
       {// C: Only filling in missing enumerators w/ typedefs
        Function,
        R""(
             typedef unsigned long UInteger;
             enum ControlState : UInteger;
             typedef enum ControlState ControlState;
             enum ControlState : UInteger {A,B,C};
             ControlState controlState = A;
             switch (^controlState) {case A:break;}
           )"",
        R""(
             typedef unsigned long UInteger;
             enum ControlState : UInteger;
             typedef enum ControlState ControlState;
             enum ControlState : UInteger {A,B,C};
             ControlState controlState = A;
-            switch (controlState) {case A:break;case B:case C:break;}
+            switch (controlState) {case A:break;case B:break;case C:break;}
           )"",
        "TestTU.c"},
   };
 
   for (const auto &Case : Cases) {
     Context = Case.Context;
     FileName = Case.FileName;
     EXPECT_EQ(apply(Case.TestSource), Case.ExpectedSource);
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang