[Diagnostics] Warn if enumeration type mismatch in conditional expression

[xbolva00]

Summary

• Useful warning
• GCC compatibility (GCC warns in C++ mode)

[xbolva00]

Added test file.

[xbolva00]

test/Sema/warn-conditional-emum-types-mismatch.c

19

Gcc warns here, but Clang does not warn when A != C..

So not sure here..

[aaron.ballman]

lib/Sema/SemaChecking.cpp

11264

const auto * (and below as well) since the type is spelled out in the initialization.

11272–11274

Would it make sense to use !LHSEnumType->getDecl()->hasNameForLinkage() here? It seems like that's the situation we care about.

11785

Might make sense to name the new function CheckConditionalWithEnumTypes() to match the local style better.

test/Sema/warn-conditional-emum-types-mismatch.c

15

Spurious '

19

My gut reaction is that I think Clang should warn here as well because the code pattern is confusing, but I'd also say that if there's a lot of false positives where the code is sensible, it may make sense to suppress the diagnostic. One situation I was thinking of where you could run into something like this is:

enum {
  STATUS_SUCCESS,
  STATUS_FAILURE,
  ...
  MAX_BASE_STATUS_CODE
};

enum ExtendedStatusCodes {
  STATUS_SOMETHING_INTERESTING = MAX_BASE_STATUS_CODE + 1000,
  ...
};

int whatever(void) {
  return some_condition() ? STATUS_SOMETHING_INTERESTING : STATUS_SUCCESS;
}

[xbolva00]

addressed review notes

[xbolva00]

test/Sema/warn-conditional-emum-types-mismatch.c

19

+1

[aaron.ballman]

LGTM aside from a commenting request.

test/Sema/warn-conditional-emum-types-mismatch.c

19

Can you add some comments to the test case explaining that we purposefully differ from GCC here and why?

[xbolva00]

Added comment why we differ

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_integer_constants_in_conditional_always_true : Warning<
   "converting the result of '?:' with integer constants to a boolean always "
   "evaluates to 'true'">,
   InGroup<TautologicalConstantCompare>;
 def warn_comparison_of_mixed_enum_types : Warning<
   "comparison of two values with different enumeration types"
   "%diff{ ($ and $)|}0,1">,
   InGroup<EnumCompare>;
+def warn_conditional_mixed_enum_types : Warning<
+  "enumeration type mismatch in conditional expression"
+  "%diff{ ($ and $)|}0,1">,
+  InGroup<EnumCompare>;
 def warn_comparison_of_mixed_enum_types_switch : Warning<
   "comparison of two values with different enumeration types in switch statement"
   "%diff{ ($ and $)|}0,1">,
   InGroup<EnumCompareSwitch>;
 def warn_null_in_arithmetic_operation : Warning<
   "use of NULL in arithmetic operation">,
   InGroup<NullArithmetic>;
 def warn_null_in_comparison_operation : Warning<

lib/Sema/SemaChecking.cpp

const char FirstContextCharacter =
         S.getSourceManager().getCharacterData(CC)[0];
     if (FirstContextCharacter == '{')
       return false;
   }
 
   return true;
 }
 
+static void checkConditionalWithEnumTypes(Sema &S, SourceLocation Loc,
+                                          Expr *LHS, Expr *RHS) {
+  QualType LHSStrippedType = LHS->IgnoreParenImpCasts()->getType();
+  QualType RHSStrippedType = RHS->IgnoreParenImpCasts()->getType();
+
+  const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>();
+  if (!LHSEnumType)
+    return;
+  const EnumType *RHSEnumType = RHSStrippedType->getAs<EnumType>();
+  if (!RHSEnumType)
+    return;
+
+  // Ignore anonymous enums.
+  if (!LHSEnumType->getDecl()->getIdentifier() &&
+      !LHSEnumType->getDecl()->getTypedefNameForAnonDecl())
+    return;
+  if (!RHSEnumType->getDecl()->getIdentifier() &&
+      !RHSEnumType->getDecl()->getTypedefNameForAnonDecl())
+    return;
+
+  if (S.Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType))
+    return;
+
+  S.Diag(Loc, diag::warn_conditional_mixed_enum_types)
+      << LHSStrippedType << RHSStrippedType << LHS->getSourceRange()
+      << RHS->getSourceRange();
+}
+
 static void DiagnoseIntInBoolContext(Sema &S, Expr *E) {
   E = E->IgnoreParenImpCasts();
   SourceLocation ExprLoc = E->getExprLoc();
 
   if (const auto *BO = dyn_cast<BinaryOperator>(E)) {
     BinaryOperator::Opcode Opc = BO->getOpcode();
     if (Opc == BO_Shl)
       S.Diag(ExprLoc, diag::warn_left_shift_in_bool_context) << E;

 
 static void CheckConditionalOperator(Sema &S, ConditionalOperator *E,
                                      SourceLocation CC, QualType T) {
   AnalyzeImplicitConversions(S, E->getCond(), E->getQuestionLoc());
 
   bool Suspicious = false;
   CheckConditionalOperand(S, E->getTrueExpr(), T, CC, Suspicious);
   CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);
+  checkConditionalWithEnumTypes(S, E->getBeginLoc(), E->getTrueExpr(),
+                                E->getFalseExpr());
 
   if (T->isBooleanType())
     DiagnoseIntInBoolContext(S, E);
 
   // If -Wconversion would have warned about either of the candidates
   // for a signedness conversion to the context type...
   if (!Suspicious) return;
 

test/Sema/warn-conditional-emum-types-mismatch.c

+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify  %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+enum ro { A = 0x10 };
+enum rw { B = 0xFF };
+enum { C = 0x1A};
+
+int get_flag(int cond) {
+  return cond ? A : B; 
+  #ifdef __cplusplus
+  // expected-warning@-2 {{enumeration type mismatch in conditional expression ('ro' and 'rw')}}
+  #else 
+  // expected-no-diagnostics'
+  #endif
+}
+
+int get_flag_anon_enum(int cond) {
+  return cond ? A : C;
+}