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 <string>
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<EnumType>();
+ EnumT = Cond->getType()->getAsAdjusted<EnumType>();
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<DefaultStmt>(CaseList))
+ return false;
+
+ const CaseStmt *CS = dyn_cast<CaseStmt>(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<ConstantExpr>(CS->getLHS());
+ if (!CE || CE->isValueDependent())
+ return false;
+ }
- return true;
+ // Only suggest tweak if we have more enumerators than cases.
+ return I != E;
}
Expected<Tweak::Effect> 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<llvm::APSInt, 32> ExistingEnumerators;
+ for (const SwitchCase *CaseList = Switch->getSwitchCaseList(); CaseList;
+ CaseList = CaseList->getNextSwitchCase()) {
+ const CaseStmt *CS = dyn_cast<CaseStmt>(CaseList);
+ if (!CS || CS->caseStmtIsGNURange())
+ continue;
+
+ const ConstantExpr *CE = dyn_cast_or_null<ConstantExpr>(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)));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
