[Sema] Don't mark plain MS enums as fixed

[rnk]

Summary

This fixes a flaw in our AST: PR27098

MSVC always gives plain enums the underlying type 'int'. Clang does this
as well, but we claim the enum is "fixed", as if the user actually wrote
': int'. It means we end up emitting spurious -Wsign-compare warnings on
code like this:

enum Vals { E1, E2, E3 };
bool f(unsigned v1, Vals v2) {
  return v1 == v2;
}

We think 'v2' can take on negative values because we think 'Vals' is
fixed. This fixes that.

[dberris]

LGTM

[smeenai]

We've seen lots of spurious warnings from this. Thanks for the fix!

[thakis]

Nice!

Test?

[rnk]

• Add test, update comments

[rnk]

In D43110#1003171, @thakis wrote:

Nice!

Test?

Yeah, I forgot to get that in. From the test you can see that we have weirdly different behavior in C and C++ mode. Do people care? This is pre-existing behavior. My change makes the enum "unfixed" in C++, which changes the C++ behavior. Changing C mode to match C++ mode seems like it should be a separate change. Do people think we should do that as well?

[rsmith]

Thanks! I'd noticed this weirdness but wasn't sure what we could do about it without breaking MS compat. I like this approach a lot.

If we want to change the C behavior too, I think that should be a separate change. How does MSVC behave in C mode?

[rnk]

In D43110#1004107, @rsmith wrote:

Thanks! I'd noticed this weirdness but wasn't sure what we could do about it without breaking MS compat. I like this approach a lot.

Great, sorry for the delay.

If we want to change the C behavior too, I think that should be a separate change. How does MSVC behave in C mode?

They warn in both C and C++ on the code below:

enum PosEnum { A_a = 0, A_b = 1, A_c = 10 };
int test_pos(enum PosEnum a, unsigned uv) { return a < uv; }

I picked this back up because clang-cl was giving -Wsign-comparison warnings in code like this:

unsigned DiagID = ...;
EXPECT_EQ(DiagID, diag::warn_foo);

I think there has been a recent growth in gtest usage, so this was starting to get really annoying. The source-level fix would be to give the diag enums a fixed type, but I didn't feel like doing that to literally every enum in LLVM and clang. I guess the MSVC build will just be noisy unless we disable that warning.

include/clang/AST/Decl.h

public:
   /// \brief Returns true if this is an Objective-C, C++11, or
   /// Microsoft-style enumeration with a fixed underlying type.
   bool isFixed() const {
     return IsFixed;
   }
 
   /// \brief Returns true if this can be considered a complete type.
   bool isComplete() const {
-    return isCompleteDefinition() || isFixed();
+    // IntegerType is set for fixed type enums and non-fixed but implicitly
+    // int-sized Microsoft enums.
+    return isCompleteDefinition() || IntegerType;
   }
 
   /// Returns true if this enum is either annotated with
   /// enum_extensibility(closed) or isn't annotated with enum_extensibility.
   bool isClosed() const;
 
   /// Returns true if this enum is annotated with flag_enum and isn't annotated
   /// with enum_extensibility(open).

include/clang/Sema/Sema.h

public:
 
   EnumConstantDecl *CheckEnumConstant(EnumDecl *Enum,
                                       EnumConstantDecl *LastEnumConst,
                                       SourceLocation IdLoc,
                                       IdentifierInfo *Id,
                                       Expr *val);
   bool CheckEnumUnderlyingType(TypeSourceInfo *TI);
   bool CheckEnumRedeclaration(SourceLocation EnumLoc, bool IsScoped,
-                              QualType EnumUnderlyingTy,
-                              bool EnumUnderlyingIsImplicit,
+                              QualType EnumUnderlyingTy, bool IsFixed,
                               const EnumDecl *Prev);
 
   /// Determine whether the body of an anonymous enumeration should be skipped.
   /// \param II The name of the first enumerator.
   SkipBodyInfo shouldSkipAnonEnumBody(Scope *S, IdentifierInfo *II,
                                       SourceLocation IILoc);
 
   Decl *ActOnEnumConstant(Scope *S, Decl *EnumDecl, Decl *LastEnumConstant,

lib/AST/Type.cpp

bool Type::isIncompleteType(NamedDecl **Def) const {
   case Builtin:
     // Void is the only incomplete builtin type.  Per C99 6.2.5p19, it can never
     // be completed.
     return isVoidType();
   case Enum: {
     EnumDecl *EnumD = cast<EnumType>(CanonicalType)->getDecl();
     if (Def)
       *Def = EnumD;
-    
-    // An enumeration with fixed underlying type is complete (C++0x 7.2p3).
-    if (EnumD->isFixed())
-      return false;
-    
-    return !EnumD->isCompleteDefinition();
+    return !EnumD->isComplete();
   }
   case Record: {
     // A tagged type (struct/union/enum/class) is incomplete if the decl is a
     // forward declaration, but not a full definition (C99 6.2.5p22).
     RecordDecl *Rec = cast<RecordType>(CanonicalType)->getDecl();
     if (Def)
       *Def = Rec;
     return !Rec->isCompleteDefinition();

lib/Sema/SemaDecl.cpp

if (BT->isInteger())
       return false;
 
   Diag(UnderlyingLoc, diag::err_enum_invalid_underlying) << T;
   return true;
 }
 
 /// Check whether this is a valid redeclaration of a previous enumeration.
 /// \return true if the redeclaration was invalid.
-bool Sema::CheckEnumRedeclaration(
-    SourceLocation EnumLoc, bool IsScoped, QualType EnumUnderlyingTy,
-    bool EnumUnderlyingIsImplicit, const EnumDecl *Prev) {
-  bool IsFixed = !EnumUnderlyingTy.isNull();
-
+bool Sema::CheckEnumRedeclaration(SourceLocation EnumLoc, bool IsScoped,
+                                  QualType EnumUnderlyingTy, bool IsFixed,
+                                  const EnumDecl *Prev) {
   if (IsScoped != Prev->isScoped()) {
     Diag(EnumLoc, diag::err_enum_redeclare_scoped_mismatch)
       << Prev->isScoped();
     Diag(Prev->getLocation(), diag::note_previous_declaration);
     return true;
   }
 
   if (IsFixed && Prev->isFixed()) {
     if (!EnumUnderlyingTy->isDependentType() &&
         !Prev->getIntegerType()->isDependentType() &&
         !Context.hasSameUnqualifiedType(EnumUnderlyingTy,
                                         Prev->getIntegerType())) {
       // TODO: Highlight the underlying type of the redeclaration.
       Diag(EnumLoc, diag::err_enum_redeclare_type_mismatch)
         << EnumUnderlyingTy << Prev->getIntegerType();
       Diag(Prev->getLocation(), diag::note_previous_declaration)
           << Prev->getIntegerTypeRange();
       return true;
     }
-  } else if (IsFixed && !Prev->isFixed() && EnumUnderlyingIsImplicit) {
-    ;
-  } else if (!IsFixed && Prev->isFixed() && !Prev->getIntegerTypeSourceInfo()) {
-    ;
   } else if (IsFixed != Prev->isFixed()) {
     Diag(EnumLoc, diag::err_enum_redeclare_fixed_mismatch)
       << Prev->isFixed();
     Diag(Prev->getLocation(), diag::note_previous_declaration);
     return true;
   }
 
   return false;
if (TemplateParameterList *TemplateParams =
       }
     }
   }
 
   // Figure out the underlying type if this a enum declaration. We need to do
   // this early, because it's needed to detect if this is an incompatible
   // redeclaration.
   llvm::PointerUnion<const Type*, TypeSourceInfo*> EnumUnderlying;
-  bool EnumUnderlyingIsImplicit = false;
+  bool IsFixed = !UnderlyingType.isUnset() || ScopedEnum;
 
   if (Kind == TTK_Enum) {
-    if (UnderlyingType.isInvalid() || (!UnderlyingType.get() && ScopedEnum))
+    if (UnderlyingType.isInvalid() || (!UnderlyingType.get() && ScopedEnum)) {
       // No underlying type explicitly specified, or we failed to parse the
       // type, default to int.
       EnumUnderlying = Context.IntTy.getTypePtr();
-    else if (UnderlyingType.get()) {
+    } else if (UnderlyingType.get()) {
       // C++0x 7.2p2: The type-specifier-seq of an enum-base shall name an
       // integral type; any cv-qualification is ignored.
       TypeSourceInfo *TI = nullptr;
       GetTypeFromParser(UnderlyingType.get(), &TI);
       EnumUnderlying = TI;
 
       if (CheckEnumUnderlyingType(TI))
         // Recover by falling back to int.
         EnumUnderlying = Context.IntTy.getTypePtr();
 
       if (DiagnoseUnexpandedParameterPack(TI->getTypeLoc().getBeginLoc(), TI,
                                           UPPC_FixedUnderlyingType))
         EnumUnderlying = Context.IntTy.getTypePtr();
 
     } else if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
-      if (getLangOpts().MSVCCompat || TUK == TUK_Definition) {
-        // Microsoft enums are always of int type.
+      // For MSVC ABI compatibility, unfixed enums must use an underlying type
+      // of 'int'. However, if this is an unfixed forward declaration, don't set
+      // the underlying type unless the user enables -fms-compatibility. This
+      // makes unfixed forward declared enums incomplete and is more conforming.
+      if (TUK == TUK_Definition || getLangOpts().MSVCCompat)
         EnumUnderlying = Context.IntTy.getTypePtr();
-        EnumUnderlyingIsImplicit = true;
-      }
     }
   }
 
   DeclContext *SearchDC = CurContext;
   DeclContext *DC = CurContext;
   bool isStdBadAlloc = false;
   bool isStdAlignValT = false;
 
auto createTagFromNewDecl = [&]() -> TagDecl * {
     // If there is an identifier, use the location of the identifier as the
     // location of the decl, otherwise use the location of the struct/union
     // keyword.
     SourceLocation Loc = NameLoc.isValid() ? NameLoc : KWLoc;
     TagDecl *New = nullptr;
 
     if (Kind == TTK_Enum) {
       New = EnumDecl::Create(Context, SearchDC, KWLoc, Loc, Name, nullptr,
-                             ScopedEnum, ScopedEnumUsesClassTag,
-                             !EnumUnderlying.isNull());
+                             ScopedEnum, ScopedEnumUsesClassTag, IsFixed);
       // If this is an undefined enum, bail.
       if (TUK != TUK_Definition && !Invalid)
         return nullptr;
       if (EnumUnderlying) {
         EnumDecl *ED = cast<EnumDecl>(New);
         if (TypeSourceInfo *TI = EnumUnderlying.dyn_cast<TypeSourceInfo *>())
           ED->setIntegerTypeSourceInfo(TI);
         else
if (TagDecl *PrevTagDecl = dyn_cast<TagDecl>(PrevDecl)) {
           else if (const Type *T = EnumUnderlying.dyn_cast<const Type*>())
             EnumUnderlyingTy = QualType(T, 0);
 
           // All conflicts with previous declarations are recovered by
           // returning the previous declaration, unless this is a definition,
           // in which case we want the caller to bail out.
           if (CheckEnumRedeclaration(NameLoc.isValid() ? NameLoc : KWLoc,
                                      ScopedEnum, EnumUnderlyingTy,
-                                     EnumUnderlyingIsImplicit, PrevEnum))
+                                     IsFixed, PrevEnum))
             return TUK == TUK_Declaration ? PrevTagDecl : nullptr;
         }
 
         // C++11 [class.mem]p1:
         //   A member shall not be declared twice in the member-specification,
         //   except that a nested class or member class template can be declared
         //   and then later defined.
         if (TUK == TUK_Declaration && PrevDecl->isCXXClassMember() &&
CreateNewDecl:
   TagDecl *New;
 
   bool IsForwardReference = false;
   if (Kind == TTK_Enum) {
     // FIXME: Tag decls should be chained to any simultaneous vardecls, e.g.:
     // enum X { A, B, C } D;    D should chain to X.
     New = EnumDecl::Create(Context, SearchDC, KWLoc, Loc, Name,
                            cast_or_null<EnumDecl>(PrevDecl), ScopedEnum,
-                           ScopedEnumUsesClassTag, !EnumUnderlying.isNull());
+                           ScopedEnumUsesClassTag, IsFixed);
 
     if (isStdAlignValT && (!StdAlignValT || getStdAlignValT()->isImplicit()))
       StdAlignValT = cast<EnumDecl>(New);
 
     // If this is an undefined enum, warn.
     if (TUK != TUK_Definition && !Invalid) {
       TagDecl *Def;
-      if (!EnumUnderlyingIsImplicit &&
-          (getLangOpts().CPlusPlus11 || getLangOpts().ObjC2) &&
+      if (IsFixed && (getLangOpts().CPlusPlus11 || getLangOpts().ObjC2) &&
           cast<EnumDecl>(New)->isFixed()) {
         // C++0x: 7.2p2: opaque-enum-declaration.
         // Conflicts are diagnosed above. Do nothing.
       }
       else if (PrevDecl && (Def = cast<EnumDecl>(PrevDecl)->getDefinition())) {
         Diag(Loc, diag::ext_forward_ref_enum_def)
           << New;
         Diag(Def->getLocation(), diag::note_previous_definition);
if (Kind == TTK_Enum) {
 
     if (EnumUnderlying) {
       EnumDecl *ED = cast<EnumDecl>(New);
       if (TypeSourceInfo *TI = EnumUnderlying.dyn_cast<TypeSourceInfo*>())
         ED->setIntegerTypeSourceInfo(TI);
       else
         ED->setIntegerType(QualType(EnumUnderlying.get<const Type*>(), 0));
       ED->setPromotionType(ED->getIntegerType());
+      assert(ED->isComplete() && "enum with type should be complete");
     }
   } else {
     // struct/union/class
 
     // FIXME: Tag decls should be chained to any simultaneous vardecls, e.g.:
     // struct X { int A; } D;    D should chain to X.
     if (getLangOpts().CPlusPlus) {
       // FIXME: Look for a way to use RecordDecl for simple structs.
else {
           Val = nullptr;
         else
           Val = Converted.get();
       } else if (!Val->isValueDependent() &&
                  !(Val = VerifyIntegerConstantExpression(Val,
                                                          &EnumVal).get())) {
         // C99 6.7.2.2p2: Make sure we have an integer constant expression.
       } else {
-        if (Enum->isFixed()) {
+        if (Enum->isComplete()) {
           EltTy = Enum->getIntegerType();
 
           // In Obj-C and Microsoft mode, require the enumeration value to be
           // representable in the underlying type of the enumeration. In C++11,
           // we perform a non-narrowing conversion as part of converted constant
           // expression checking.
           if (!isRepresentableIntegerValue(Context, EnumVal, EltTy)) {
             if (getLangOpts().MSVCCompat) {
void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
   QualType BestPromotionType;
 
   bool Packed = Enum->hasAttr<PackedAttr>();
   // -fshort-enums is the equivalent to specifying the packed attribute on all
   // enum definitions.
   if (LangOpts.ShortEnums)
     Packed = true;
 
-  if (Enum->isFixed()) {
+  // If the enum already has a type because it is fixed or dictated by the
+  // target, promote that type instead of analyzing the enumerators.
+  if (Enum->isComplete()) {
     BestType = Enum->getIntegerType();
     if (BestType->isPromotableIntegerType())
       BestPromotionType = Context.getPromotedIntegerType(BestType);
     else
       BestPromotionType = BestType;
 
     BestWidth = Context.getIntWidth(BestType);
   }

lib/Sema/SemaTemplateInstantiateDecl.cpp

if (Def && Def != D) {
     // that the underlying types match in the instantiation of both
     // declarations.
     if (TypeSourceInfo *TI = Def->getIntegerTypeSourceInfo()) {
       SourceLocation UnderlyingLoc = TI->getTypeLoc().getBeginLoc();
       QualType DefnUnderlying =
         SemaRef.SubstType(TI->getType(), TemplateArgs,
                           UnderlyingLoc, DeclarationName());
       SemaRef.CheckEnumRedeclaration(Def->getLocation(), Def->isScoped(),
-                                     DefnUnderlying,
-                                     /*EnumUnderlyingIsImplicit=*/false, Enum);
+                                     DefnUnderlying, /*IsFixed=*/true, Enum);
     }
   }
 
   // C++11 [temp.inst]p1: The implicit instantiation of a class template
   // specialization causes the implicit instantiation of the declarations, but
   // not the definitions of scoped member enumerations.
   //
   // DR1484 clarifies that enumeration definitions inside of a template

test/Sema/sign-compare-enum.c

-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -verify -Wsign-compare %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -verify -Wsign-compare %s
-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -DSILENCE -verify %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -DSILENCE -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -Wsign-compare %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-compare %s
+// RUN: %clang_cc1 -x c++ -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -Wsign-compare %s
+// RUN: %clang_cc1 -x c++ -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-compare %s
+
+// Check that -Wsign-compare is off by default.
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -DSILENCE %s
+
+#ifdef SILENCE
+// expected-no-diagnostics
+#endif
+
+enum PosEnum {
+  A_a = 0,
+  A_b = 1,
+  A_c = 10
+};
 
-int main() {
-  enum A { A_a = 0, A_b = 1 };
-  static const int message[] = {0, 1};
-  enum A a;
 
+int test_pos(enum PosEnum a) {
   if (a < 2)
     return 0;
 
-#if defined(SIGNED) && !defined(SILENCE)
-  if (a < sizeof(message)/sizeof(message[0])) // expected-warning {{comparison of integers of different signs: 'enum A' and 'unsigned long long'}}
-    return 0;
-#else
-  // expected-no-diagnostics
+  // No warning, except in Windows C mode, where PosEnum is 'int' and it can
+  // take on any value according to the C standard.
+#if !defined(SILENCE) && defined(_WIN32) && !defined(__cplusplus)
+  // expected-warning@+2 {{comparison of integers of different signs}}
+#endif
   if (a < 2U)
     return 0;
+
+  unsigned uv = 2;
+#if !defined(SILENCE) && defined(_WIN32) && !defined(__cplusplus)
+  // expected-warning@+2 {{comparison of integers of different signs}}
+#endif
+  if (a < uv)
+    return 1;
+
+#if !defined(SILENCE) && defined(_WIN32) && !defined(__cplusplus)
+  // expected-warning@+2 {{comparison of integers of different signs}}
+#endif
   if (a < sizeof(message)/sizeof(message[0]))
     return 0;
+  return 4;
+}
+
+enum NegEnum {
+  NE_a = -1,
+  NE_b = 0,
+  NE_c = 10
+};
+
+int test_neg(enum NegEnum a) {
+  if (a < 2)
+    return 0;
+
+#ifndef SILENCE
+  // expected-warning@+2 {{comparison of integers of different signs}}
 #endif
+  if (a < 2U)
+    return 0;
+
+  unsigned uv = 2;
+#ifndef SILENCE
+  // expected-warning@+2 {{comparison of integers of different signs}}
+#endif
+  if (a < uv)
+    return 1;
+
+#ifndef SILENCE
+  // expected-warning@+2 {{comparison of integers of different signs}}
+#endif
+  if (a < sizeof(message)/sizeof(message[0]))
+    return 0;
+  return 4;
 }