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_empty()) - return false; - const Expr *Cond = Switch->getCond(); if (!Cond) return false; @@ -106,7 +101,7 @@ // Ignore implicit casts, since enums implicitly cast to integer types. Cond = Cond->IgnoreParenImpCasts(); - const EnumType *EnumT = Cond->getType()->getAsAdjusted(); + EnumT = Cond->getType()->getAsAdjusted(); if (!EnumT) return false; @@ -114,21 +109,69 @@ if (!EnumD) return false; - // If there aren't any enumerators, there's nothing to insert. - if (EnumD->enumerator_begin() == EnumD->enumerator_end()) - return false; + // Iterate through switch cases and enumerators at the same time, so we can + // exit early if we have more cases than enumerators. + auto I = EnumD->enumerator_begin(); + auto E = EnumD->enumerator_end(); + + for (const SwitchCase *CaseList = Switch->getSwitchCaseList(); + CaseList && I != E; CaseList = CaseList->getNextSwitchCase(), I++) { + // Switch has a default case. + if (isa(CaseList)) + return false; + + const CaseStmt *CS = dyn_cast(CaseList); + if (!CS) + continue; + + // Case expression is a GNU range. + if (CS->caseStmtIsGNURange()) + return false; + + // Case expression is not a constant expression or is value-dependent. + const ConstantExpr *CE = dyn_cast(CS->getLHS()); + if (!CE || CE->isValueDependent()) + return false; + } - return true; + // Only suggest tweak if we have more enumerators than cases. + return I != E; } Expected PopulateSwitch::apply(const Selection &Sel) { - const SourceManager &SM = ASTCtx->getSourceManager(); + ASTContext &Ctx = Sel.AST->getASTContext(); + + // Get the enum's integer width and signedness, for adjusting case literals. + unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0)); + bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType(); + + llvm::SmallSet ExistingEnumerators; + for (const SwitchCase *CaseList = Switch->getSwitchCaseList(); CaseList; + CaseList = CaseList->getNextSwitchCase()) { + const CaseStmt *CS = dyn_cast(CaseList); + if (!CS || CS->caseStmtIsGNURange()) + continue; + + const ConstantExpr *CE = dyn_cast_or_null(CS->getLHS()); + if (!CE || CE->isValueDependent()) + continue; + + llvm::APSInt Val = CE->getResultAsAPSInt(); + Val = Val.extOrTrunc(EnumIntWidth); + Val.setIsSigned(EnumIsSigned); + ExistingEnumerators.insert(Val); + } + SourceLocation Loc = Body->getRBracLoc(); + ASTContext &DeclASTCtx = DeclCtx->getParentASTContext(); std::string Text; for (EnumConstantDecl *Enumerator : EnumD->enumerators()) { + if (ExistingEnumerators.contains(Enumerator->getInitVal())) + continue; + Text += "case "; - Text += getQualification(*ASTCtx, DeclCtx, Loc, EnumD); + Text += getQualification(DeclASTCtx, DeclCtx, Loc, EnumD); if (EnumD->isScoped()) { Text += EnumD->getName(); Text += "::"; @@ -136,8 +179,12 @@ Text += Enumerator->getName(); Text += ":"; } + + if (Text.empty()) + return error("Populate switch: no enumerators to insert."); Text += "break;"; + const SourceManager &SM = Ctx.getSourceManager(); return Effect::mainFileEdit( SM, tooling::Replacements(tooling::Replacement(SM, Loc, 0, Text))); } 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