[clang-tools-extra] 5918ef8 - [clangd] Handle duplicate enum constants in PopulateSwitch tweak
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 9 04:15:05 PST 2020
Author: Nathan James
Date: 2020-11-09T12:14:53Z
New Revision: 5918ef8b1aac00af2f8dd8b99f3de7b176463444
URL: https://github.com/llvm/llvm-project/commit/5918ef8b1aac00af2f8dd8b99f3de7b176463444
DIFF: https://github.com/llvm/llvm-project/commit/5918ef8b1aac00af2f8dd8b99f3de7b176463444.diff
LOG: [clangd] Handle duplicate enum constants in PopulateSwitch tweak
If an enum has different names for the same constant, make sure only the first one declared gets added into the switch. Failing to do so results in a compiler error as 2 case labels can't represent the same value.
```
lang=c
enum Numbers{
One,
Un = One,
Two,
Deux = Two,
Three,
Trois = Three
};
// Old behaviour
switch (<Number>) {
case One:
case Un:
case Two:
case Duex:
case Three:
case Trois: break;
}
// New behaviour
switch (<Number>) {
case One:
case Two:
case Three: break;
}
```
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D90555
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
llvm/include/llvm/ADT/DenseMapInfo.h
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp b/clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
index a8ad90596681..852888f6a043 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -40,7 +40,8 @@
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Tooling/Core/Replacement.h"
-#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/STLExtras.h"
#include <cassert>
#include <string>
@@ -57,11 +58,27 @@ class PopulateSwitch : public Tweak {
}
private:
+ class ExpectedCase {
+ public:
+ ExpectedCase(const EnumConstantDecl *Decl) : Data(Decl, false) {}
+ bool isCovered() const { return Data.getInt(); }
+ void setCovered(bool Val = true) { Data.setInt(Val); }
+ const EnumConstantDecl *getEnumConstant() const {
+ return Data.getPointer();
+ }
+
+ private:
+ llvm::PointerIntPair<const EnumConstantDecl *, 1, bool> Data;
+ };
+
const DeclContext *DeclCtx = nullptr;
const SwitchStmt *Switch = nullptr;
const CompoundStmt *Body = nullptr;
const EnumType *EnumT = nullptr;
const EnumDecl *EnumD = nullptr;
+ // Maps the Enum values to the EnumConstantDecl and a bool signifying if its
+ // covered in the switch.
+ llvm::MapVector<llvm::APSInt, ExpectedCase> ExpectedCases;
};
REGISTER_TWEAK(PopulateSwitch)
@@ -112,21 +129,34 @@ bool PopulateSwitch::prepare(const Selection &Sel) {
if (!EnumD)
return false;
- // We trigger if there are fewer cases than enum values (and no case covers
- // multiple values). This guarantees we'll have at least one case to insert.
- // We don't yet determine what the cases are, as that means evaluating
- // expressions.
- auto I = EnumD->enumerator_begin();
- auto E = EnumD->enumerator_end();
+ // We trigger if there are any values in the enum that aren't covered by the
+ // switch.
+
+ ASTContext &Ctx = Sel.AST->getASTContext();
+
+ unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0));
+ bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType();
- for (const SwitchCase *CaseList = Switch->getSwitchCaseList();
- CaseList && I != E; CaseList = CaseList->getNextSwitchCase(), I++) {
+ auto Normalize = [&](llvm::APSInt Val) {
+ Val = Val.extOrTrunc(EnumIntWidth);
+ Val.setIsSigned(EnumIsSigned);
+ return Val;
+ };
+
+ for (auto *EnumConstant : EnumD->enumerators()) {
+ ExpectedCases.insert(
+ std::make_pair(Normalize(EnumConstant->getInitVal()), EnumConstant));
+ }
+
+ for (const SwitchCase *CaseList = Switch->getSwitchCaseList(); CaseList;
+ CaseList = CaseList->getNextSwitchCase()) {
// Default likely intends to cover cases we'd insert.
if (isa<DefaultStmt>(CaseList))
return false;
const CaseStmt *CS = cast<CaseStmt>(CaseList);
- // Case statement covers multiple values, so just counting doesn't work.
+
+ // GNU range cases are rare, we don't support them.
if (CS->caseStmtIsGNURange())
return false;
@@ -135,48 +165,36 @@ bool PopulateSwitch::prepare(const Selection &Sel) {
const ConstantExpr *CE = dyn_cast<ConstantExpr>(CS->getLHS());
if (!CE || CE->isValueDependent())
return false;
+
+ // Unsure if this case could ever come up, but prevents an unreachable
+ // executing in getResultAsAPSInt.
+ if (CE->getResultStorageKind() == ConstantExpr::RSK_None)
+ return false;
+ auto Iter = ExpectedCases.find(Normalize(CE->getResultAsAPSInt()));
+ if (Iter != ExpectedCases.end())
+ Iter->second.setCovered();
}
- // Only suggest tweak if we have more enumerators than cases.
- return I != E;
+ return !llvm::all_of(ExpectedCases,
+ [](auto &Pair) { return Pair.second.isCovered(); });
}
Expected<Tweak::Effect> PopulateSwitch::apply(const Selection &Sel) {
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 = cast<CaseStmt>(CaseList);
- assert(!CS->caseStmtIsGNURange());
- const ConstantExpr *CE = cast<ConstantExpr>(CS->getLHS());
- assert(!CE->isValueDependent());
- 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()))
+ llvm::SmallString<256> Text;
+ for (auto &EnumConstant : ExpectedCases) {
+ // Skip any enum constants already covered
+ if (EnumConstant.second.isCovered())
continue;
- Text += "case ";
- Text += getQualification(DeclASTCtx, DeclCtx, Loc, EnumD);
- if (EnumD->isScoped()) {
- Text += EnumD->getName();
- Text += "::";
- }
- Text += Enumerator->getName();
- Text += ":";
+ Text.append({"case ", getQualification(DeclASTCtx, DeclCtx, Loc, EnumD)});
+ if (EnumD->isScoped())
+ Text.append({EnumD->getName(), "::"});
+ Text.append({EnumConstant.second.getEnumConstant()->getName(), ":"});
}
assert(!Text.empty() && "No enumerators to insert!");
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index 563be79c4c0d..98d797aacd8f 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2980,6 +2980,18 @@ TEST_F(PopulateSwitchTest, Test) {
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;})"",
+ },
+ {
+ // Duplicated constant names all in switch
+ Function,
+ R""(enum Enum {A,B,b=B}; ^switch (A) {case A:case B:break;})"",
+ "unavailable",
+ },
};
for (const auto &Case : Cases) {
diff --git a/llvm/include/llvm/ADT/DenseMapInfo.h b/llvm/include/llvm/ADT/DenseMapInfo.h
index 1cace4b3192f..8271b9334b86 100644
--- a/llvm/include/llvm/ADT/DenseMapInfo.h
+++ b/llvm/include/llvm/ADT/DenseMapInfo.h
@@ -14,6 +14,7 @@
#define LLVM_ADT_DENSEMAPINFO_H
#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/StringRef.h"
@@ -371,6 +372,26 @@ template <> struct DenseMapInfo<APInt> {
}
};
+/// Provide DenseMapInfo for APSInt, using the DenseMapInfo for APInt.
+template <> struct DenseMapInfo<APSInt> {
+ static inline APSInt getEmptyKey() {
+ return APSInt(DenseMapInfo<APInt>::getEmptyKey());
+ }
+
+ static inline APSInt getTombstoneKey() {
+ return APSInt(DenseMapInfo<APInt>::getTombstoneKey());
+ }
+
+ static unsigned getHashValue(const APSInt &Key) {
+ return static_cast<unsigned>(hash_value(Key));
+ }
+
+ static bool isEqual(const APSInt &LHS, const APSInt &RHS) {
+ return LHS.getBitWidth() == RHS.getBitWidth() &&
+ LHS.isUnsigned() == RHS.isUnsigned() && LHS == RHS;
+ }
+};
+
} // end namespace llvm
#endif // LLVM_ADT_DENSEMAPINFO_H
More information about the cfe-commits
mailing list