[PATCH] D67919: [Diagnostics] Warn if enumeration type mismatch in conditional expression

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:11264
+
+  const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>();
+  if (!LHSEnumType)
----------------
`const auto *` (and below as well) since the type is spelled out in the initialization.


================
Comment at: lib/Sema/SemaChecking.cpp:11272-11274
+  if (!LHSEnumType->getDecl()->getIdentifier() &&
+      !LHSEnumType->getDecl()->getTypedefNameForAnonDecl())
+    return;
----------------
Would it make sense to use `!LHSEnumType->getDecl()->hasNameForLinkage()` here? It seems like that's the situation we care about.


================
Comment at: lib/Sema/SemaChecking.cpp:11785
   CheckConditionalOperand(S, E->getFalseExpr(), T, CC, Suspicious);
+  checkConditionalWithEnumTypes(S, E->getBeginLoc(), E->getTrueExpr(),
+                                E->getFalseExpr());
----------------
Might make sense to name the new function `CheckConditionalWithEnumTypes()` to match the local style better.


================
Comment at: test/Sema/warn-conditional-emum-types-mismatch.c:15
+  #else 
+  // expected-no-diagnostics'
+  #endif
----------------
Spurious `'`


================
Comment at: test/Sema/warn-conditional-emum-types-mismatch.c:19
+
+int get_flag_anon_enum(int cond) {
+  return cond ? A : C;
----------------
xbolva00 wrote:
> Gcc warns here, but Clang does not warn when A != C..
> 
> So not sure here..
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;
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67919/new/

https://reviews.llvm.org/D67919