[cfe-commits] r164083 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaDecl.cpp test/Sema/warn-duplicate-enum.c test/SemaCXX/warn-unique-enum.cpp
Ted Kremenek
kremenek at apple.com
Fri Dec 21 17:22:32 PST 2012
Hi Richard,
I was looking through my unread messages, and spotted this one. I'm so sorry for missing this.
I'm trying to page this back in now, and it all centered around the following thread on cfe-dev:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-September/024191.html
That thread clearly discusses -Wunique-enum. I've long paged this out, so I tried to look back and the thread and the original complaints I had received against -Wunique-enum from users and I don't see anything about -Wduplicate-enum. I honestly may have intended to rip it out, or maybe I just got confused because the names were so close. Since I don't know, I think it's reasonable to put the warning back in. If we get user feedback that specifically complains about -Wduplicate-enum (instead of -Wunique-enum), we can act based on concrete information.
Ted
On Nov 1, 2012, at 2:08 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Wed, Oct 3, 2012 at 2:27 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Mon, Sep 17, 2012 at 5:41 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Mon Sep 17 19:41:42 2012
> New Revision: 164083
>
> URL: http://llvm.org/viewvc/llvm-project?rev=164083&view=rev
> Log:
> Per discussion on cfe-dev, remove -Wunique-enums entirely. There
> is no compelling argument that this is a generally useful warning,
> and imposes a strong stylistic argument on code beyond what it was
> intended to find warnings in.
>
> Removed:
> cfe/trunk/test/Sema/warn-duplicate-enum.c
> cfe/trunk/test/SemaCXX/warn-unique-enum.cpp
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaDecl.cpp
>
> Hey Ted,
>
> Why was -Wduplicate-enum also removed in this commit?
>
> Richard.
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=164083&r1=164082&r2=164083&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Sep 17 19:41:42 2012
> @@ -20,17 +20,6 @@
> "used in loop condition not modified in loop body">,
> InGroup<DiagGroup<"loop-analysis">>, DefaultIgnore;
>
> -def warn_identical_enum_values : Warning<
> - "all elements of %0 are initialized with literals to value %1">,
> - InGroup<DiagGroup<"unique-enum">>;
> -def note_identical_enum_values : Note<
> - "initialize the last element with the previous element to silence "
> - "this warning">;
> -def warn_duplicate_enum_values : Warning<
> - "element %0 has been implicitly assigned %1 which another element has "
> - "been assigned">, InGroup<DiagGroup<"duplicate-enum">>, DefaultIgnore;
> -def note_duplicate_element : Note<"element %0 also has value %1">;
> -
> // Constant expressions
> def err_expr_not_ice : Error<
> "expression is not an %select{integer|integral}0 constant expression">;
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=164083&r1=164082&r2=164083&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Mon Sep 17 19:41:42 2012
> @@ -10521,233 +10521,6 @@
> return New;
> }
>
> -// Emits a warning if every element in the enum is the same value and if
> -// every element is initialized with a integer or boolean literal.
> -static void CheckForUniqueEnumValues(Sema &S, Decl **Elements,
> - unsigned NumElements, EnumDecl *Enum,
> - QualType EnumType) {
> - if (S.Diags.getDiagnosticLevel(diag::warn_identical_enum_values,
> - Enum->getLocation()) ==
> - DiagnosticsEngine::Ignored)
> - return;
> -
> - if (NumElements < 2)
> - return;
> -
> - if (!Enum->getIdentifier())
> - return;
> -
> - llvm::APSInt FirstVal;
> -
> - for (unsigned i = 0; i != NumElements; ++i) {
> - EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(Elements[i]);
> - if (!ECD)
> - return;
> -
> - Expr *InitExpr = ECD->getInitExpr();
> - if (!InitExpr)
> - return;
> - InitExpr = InitExpr->IgnoreImpCasts();
> - if (!isa<IntegerLiteral>(InitExpr) && !isa<CXXBoolLiteralExpr>(InitExpr))
> - return;
> -
> - if (i == 0) {
> - FirstVal = ECD->getInitVal();
> - continue;
> - }
> -
> - if (!llvm::APSInt::isSameValue(FirstVal, ECD->getInitVal()))
> - return;
> - }
> -
> - S.Diag(Enum->getLocation(), diag::warn_identical_enum_values)
> - << EnumType << FirstVal.toString(10)
> - << Enum->getSourceRange();
> -
> - EnumConstantDecl *Last = cast<EnumConstantDecl>(Elements[NumElements - 1]),
> - *Next = cast<EnumConstantDecl>(Elements[NumElements - 2]);
> -
> - S.Diag(Last->getLocation(), diag::note_identical_enum_values)
> - << FixItHint::CreateReplacement(Last->getInitExpr()->getSourceRange(),
> - Next->getName());
> -}
> -
> -// Returns true when the enum initial expression does not trigger the
> -// duplicate enum warning. A few common cases are exempted as follows:
> -// Element2 = Element1
> -// Element2 = Element1 + 1
> -// Element2 = Element1 - 1
> -// Where Element2 and Element1 are from the same enum.
> -static bool ValidDuplicateEnum(EnumConstantDecl *ECD, EnumDecl *Enum) {
> - Expr *InitExpr = ECD->getInitExpr();
> - if (!InitExpr)
> - return true;
> - InitExpr = InitExpr->IgnoreImpCasts();
> -
> - if (BinaryOperator *BO = dyn_cast<BinaryOperator>(InitExpr)) {
> - if (!BO->isAdditiveOp())
> - return true;
> - IntegerLiteral *IL = dyn_cast<IntegerLiteral>(BO->getRHS());
> - if (!IL)
> - return true;
> - if (IL->getValue() != 1)
> - return true;
> -
> - InitExpr = BO->getLHS();
> - }
> -
> - // This checks if the elements are from the same enum.
> - DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InitExpr);
> - if (!DRE)
> - return true;
> -
> - EnumConstantDecl *EnumConstant = dyn_cast<EnumConstantDecl>(DRE->getDecl());
> - if (!EnumConstant)
> - return true;
> -
> - if (cast<EnumDecl>(TagDecl::castFromDeclContext(ECD->getDeclContext())) !=
> - Enum)
> - return true;
> -
> - return false;
> -}
> -
> -struct DupKey {
> - int64_t val;
> - bool isTombstoneOrEmptyKey;
> - DupKey(int64_t val, bool isTombstoneOrEmptyKey)
> - : val(val), isTombstoneOrEmptyKey(isTombstoneOrEmptyKey) {}
> -};
> -
> -static DupKey GetDupKey(const llvm::APSInt& Val) {
> - return DupKey(Val.isSigned() ? Val.getSExtValue() : Val.getZExtValue(),
> - false);
> -}
> -
> -struct DenseMapInfoDupKey {
> - static DupKey getEmptyKey() { return DupKey(0, true); }
> - static DupKey getTombstoneKey() { return DupKey(1, true); }
> - static unsigned getHashValue(const DupKey Key) {
> - return (unsigned)(Key.val * 37);
> - }
> - static bool isEqual(const DupKey& LHS, const DupKey& RHS) {
> - return LHS.isTombstoneOrEmptyKey == RHS.isTombstoneOrEmptyKey &&
> - LHS.val == RHS.val;
> - }
> -};
> -
> -// Emits a warning when an element is implicitly set a value that
> -// a previous element has already been set to.
> -static void CheckForDuplicateEnumValues(Sema &S, Decl **Elements,
> - unsigned NumElements, EnumDecl *Enum,
> - QualType EnumType) {
> - if (S.Diags.getDiagnosticLevel(diag::warn_duplicate_enum_values,
> - Enum->getLocation()) ==
> - DiagnosticsEngine::Ignored)
> - return;
> - // Avoid anonymous enums
> - if (!Enum->getIdentifier())
> - return;
> -
> - // Only check for small enums.
> - if (Enum->getNumPositiveBits() > 63 || Enum->getNumNegativeBits() > 64)
> - return;
> -
> - typedef llvm::SmallVector<EnumConstantDecl*, 3> ECDVector;
> - typedef llvm::SmallVector<ECDVector*, 3> DuplicatesVector;
> -
> - typedef llvm::PointerUnion<EnumConstantDecl*, ECDVector*> DeclOrVector;
> - typedef llvm::DenseMap<DupKey, DeclOrVector, DenseMapInfoDupKey>
> - ValueToVectorMap;
> -
> - DuplicatesVector DupVector;
> - ValueToVectorMap EnumMap;
> -
> - // Populate the EnumMap with all values represented by enum constants without
> - // an initialier.
> - for (unsigned i = 0; i < NumElements; ++i) {
> - EnumConstantDecl *ECD = cast<EnumConstantDecl>(Elements[i]);
> -
> - // Null EnumConstantDecl means a previous diagnostic has been emitted for
> - // this constant. Skip this enum since it may be ill-formed.
> - if (!ECD) {
> - return;
> - }
> -
> - if (ECD->getInitExpr())
> - continue;
> -
> - DupKey Key = GetDupKey(ECD->getInitVal());
> - DeclOrVector &Entry = EnumMap[Key];
> -
> - // First time encountering this value.
> - if (Entry.isNull())
> - Entry = ECD;
> - }
> -
> - // Create vectors for any values that has duplicates.
> - for (unsigned i = 0; i < NumElements; ++i) {
> - EnumConstantDecl *ECD = cast<EnumConstantDecl>(Elements[i]);
> - if (!ValidDuplicateEnum(ECD, Enum))
> - continue;
> -
> - DupKey Key = GetDupKey(ECD->getInitVal());
> -
> - DeclOrVector& Entry = EnumMap[Key];
> - if (Entry.isNull())
> - continue;
> -
> - if (EnumConstantDecl *D = Entry.dyn_cast<EnumConstantDecl*>()) {
> - // Ensure constants are different.
> - if (D == ECD)
> - continue;
> -
> - // Create new vector and push values onto it.
> - ECDVector *Vec = new ECDVector();
> - Vec->push_back(D);
> - Vec->push_back(ECD);
> -
> - // Update entry to point to the duplicates vector.
> - Entry = Vec;
> -
> - // Store the vector somewhere we can consult later for quick emission of
> - // diagnostics.
> - DupVector.push_back(Vec);
> - continue;
> - }
> -
> - ECDVector *Vec = Entry.get<ECDVector*>();
> - // Make sure constants are not added more than once.
> - if (*Vec->begin() == ECD)
> - continue;
> -
> - Vec->push_back(ECD);
> - }
> -
> - // Emit diagnostics.
> - for (DuplicatesVector::iterator DupVectorIter = DupVector.begin(),
> - DupVectorEnd = DupVector.end();
> - DupVectorIter != DupVectorEnd; ++DupVectorIter) {
> - ECDVector *Vec = *DupVectorIter;
> - assert(Vec->size() > 1 && "ECDVector should have at least 2 elements.");
> -
> - // Emit warning for one enum constant.
> - ECDVector::iterator I = Vec->begin();
> - S.Diag((*I)->getLocation(), diag::warn_duplicate_enum_values)
> - << (*I)->getName() << (*I)->getInitVal().toString(10)
> - << (*I)->getSourceRange();
> - ++I;
> -
> - // Emit one note for each of the remaining enum constants with
> - // the same value.
> - for (ECDVector::iterator E = Vec->end(); I != E; ++I)
> - S.Diag((*I)->getLocation(), diag::note_duplicate_element)
> - << (*I)->getName() << (*I)->getInitVal().toString(10)
> - << (*I)->getSourceRange();
> - delete Vec;
> - }
> -}
> -
> void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceLocation LBraceLoc,
> SourceLocation RBraceLoc, Decl *EnumDeclX,
> Decl **Elements, unsigned NumElements,
> @@ -10970,9 +10743,6 @@
> // it needs to go into the function scope.
> if (InFunctionDeclarator)
> DeclsInPrototypeScope.push_back(Enum);
> -
> - CheckForUniqueEnumValues(*this, Elements, NumElements, Enum, EnumType);
> - CheckForDuplicateEnumValues(*this, Elements, NumElements, Enum, EnumType);
> }
>
> Decl *Sema::ActOnFileScopeAsmDecl(Expr *expr,
>
> Removed: cfe/trunk/test/Sema/warn-duplicate-enum.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-duplicate-enum.c?rev=164082&view=auto
> ==============================================================================
> --- cfe/trunk/test/Sema/warn-duplicate-enum.c (original)
> +++ cfe/trunk/test/Sema/warn-duplicate-enum.c (removed)
> @@ -1,92 +0,0 @@
> -// RUN: %clang_cc1 %s -fsyntax-only -verify -Wduplicate-enum
> -// RUN: %clang_cc1 %s -x c++ -fsyntax-only -verify -Wduplicate-enum
> -enum A {
> - A1 = 0, // expected-note {{element A1 also has value 0}}
> - A2 = -1,
> - A3, // expected-warning {{element A3 has been implicitly assigned 0 which another element has been assigned}}
> - A4};
> -
> -enum B {
> - B1 = -1, // expected-note {{element B1 also has value -1}}
> - B2, // expected-warning {{element B2 has been implicitly assigned 0 which another element has been assigned}}
> - B3,
> - B4 = -2,
> - B5, // expected-warning {{element B5 has been implicitly assigned -1 which another element has been assigned}}
> - B6 // expected-note {{element B6 also has value 0}}
> -};
> -
> -enum C { C1, C2 = -1, C3 }; // expected-warning{{element C1 has been implicitly assigned 0 which another element has been assigned}} \
> - // expected-note {{element C3 also has value 0}}
> -
> -enum D {
> - D1,
> - D2,
> - D3, // expected-warning{{element D3 has been implicitly assigned 2 which another element has been assigned}}
> - D4 = D2, // no warning
> - D5 = 2 // expected-note {{element D5 also has value 2}}
> -};
> -
> -enum E {
> - E1,
> - E2 = E1,
> - E3 = E2
> -};
> -
> -enum F {
> - F1,
> - F2,
> - FCount,
> - FMax = FCount - 1
> -};
> -
> -enum G {
> - G1,
> - G2,
> - GMax = G2,
> - GCount = GMax + 1
> -};
> -
> -enum {
> - H1 = 0,
> - H2 = -1,
> - H3,
> - H4};
> -
> -enum {
> - I1 = -1,
> - I2,
> - I3,
> - I4 = -2,
> - I5,
> - I6
> -};
> -
> -enum { J1, J2 = -1, J3 };
> -
> -enum {
> - K1,
> - K2,
> - K3,
> - K4 = K2,
> - K5 = 2
> -};
> -
> -enum {
> - L1,
> - L2 = L1,
> - L3 = L2
> -};
> -
> -enum {
> - M1,
> - M2,
> - MCount,
> - MMax = MCount - 1
> -};
> -
> -enum {
> - N1,
> - N2,
> - NMax = N2,
> - NCount = NMax + 1
> -};
>
> Removed: cfe/trunk/test/SemaCXX/warn-unique-enum.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-unique-enum.cpp?rev=164082&view=auto
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/warn-unique-enum.cpp (original)
> +++ cfe/trunk/test/SemaCXX/warn-unique-enum.cpp (removed)
> @@ -1,27 +0,0 @@
> -// RUN: %clang_cc1 %s -fsyntax-only -verify -Wunique-enum
> -enum A { A1 = 1, A2 = 1, A3 = 1 }; // expected-warning {{all elements of 'A' are initialized with literals to value 1}} \
> -// expected-note {{initialize the last element with the previous element to silence this warning}}
> -enum { B1 = 1, B2 = 1, B3 = 1 }; // no warning
> -enum C { // expected-warning {{all elements of 'C' are initialized with literals to value 1}}
> - C1 = true,
> - C2 = true // expected-note {{initialize the last element with the previous element to silence this warning}}
> -};
> -enum D { D1 = 5, D2 = 5L, D3 = 5UL, D4 = 5LL, D5 = 5ULL }; // expected-warning {{all elements of 'D' are initialized with literals to value 5}} \
> -// expected-note {{initialize the last element with the previous element to silence this warning}}
> -
> -// Don't warn on enums with less than 2 elements.
> -enum E { E1 = 4 };
> -enum F { F1 };
> -enum G {};
> -
> -// Don't warn when integer literals do not initialize the elements.
> -enum H { H1 = 4, H_MAX = H1, H_MIN = H1 };
> -enum I { I1 = H1, I2 = 4 };
> -enum J { J1 = 4, J2 = I2 };
> -enum K { K1, K2, K3, K4 };
> -
> -// Don't crash or warn on this one.
> -// rdar://11875995
> -enum L {
> - L1 = 0x8000000000000000ULL, L2 = 0x0000000000000001ULL
> -};
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> Why was -Wduplicate-enum removed? It is a separate warning from -Wunique-enum.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121221/bba9762d/attachment.html>
More information about the cfe-commits
mailing list