[PATCH] D39122: [Sema] Fixes for enum handling for tautological comparison diagnostics

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete1111 added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8610
 
+  if(!S.getLangOpts().CPlusPlus && OtherT->isEnumeralType()) {
+    OtherT = OtherT->getAs<EnumType>()->getDecl()->getIntegerType();
----------------
Please drop the braces.


================
Comment at: lib/Sema/SemaChecking.cpp:8616
 
+  // Special-case for C++ for enum with one enumerator with value of 0
+  if (OtherRange.Width == 0)
----------------
You missed a dot at the end.


================
Comment at: lib/Sema/SemaChecking.cpp:8665
 
+  bool Result;
   bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant;
----------------
You can define `Result` lower down (and make it `const`).


================
Comment at: lib/Sema/SemaChecking.cpp:8713
+  QualType WrittenType = OtherT;
+  if(!S.getLangOpts().CPlusPlus && OtherT->isEnumeralType()) {
+    OtherT = OtherT->getAs<EnumType>()->getDecl()->getIntegerType();
----------------
Same as before. Also, shouldn't this be a function instead of duplicating the same code two times?


================
Comment at: test/Sema/outof-range-enum-constant-compare.c:1
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -DUNSIGNED -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -DSIGNED -verify %s
----------------
I don't know what the convention is, but I would prefer to use platform independent tests wherever possible. I couldn't find a flag to change the underlying type of an enum, so I'm not sure if my suggestion is even feasible.


Repository:
  rL LLVM

https://reviews.llvm.org/D39122