[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
Richard Trieu
rtrieu at google.com
Fri Dec 21 18:04:18 PST 2012
Thanks Ted. I can see how these two warnings would get confused. I did
work on both around the same time, both deal with enums and are adjacent to
each other in code. I do hope that users find this warning useful.
On Fri, Dec 21, 2012 at 5:22 PM, Ted Kremenek <kremenek at apple.com> wrote:
> 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/8f1404bc/attachment.html>
More information about the cfe-commits
mailing list