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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8186
+      // For enum types, for C code, use underlying data type.
+      if (const EnumType *ET = dyn_cast<EnumType>(T))
+        T = ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
----------------
aaron.ballman wrote:
> Can use `const auto *` here.
I do agree that as per [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | coding style, that is preferred ]]  but the rest of the class would still use the
explicitly-spelled types, and IMO not deviating from local coding style in small changes might be best?

(clang-tidy does not complain about this, but maybe that is because it is not aware of this LLVM internal type casting functions)

But if you insist i will change it.


================
Comment at: lib/Sema/SemaChecking.cpp:8593
+  Min, // e.g. -32768 for short
+  Both // e.g. in C++, A::a in enum A { a = 0 };
 };
----------------
aaron.ballman wrote:
> `Both` is not very descriptive -- I think `Zero` might be an improvement.
But then it will be confusing what is the difference between `Zero` and `Min` for `unsigned` types.
I think i should just clarify the comment here.


================
Comment at: lib/Sema/SemaChecking.cpp:8612
+    OtherT = OtherT->getAs<EnumType>()->getDecl()->getIntegerType();
+  }
+
----------------
efriedma wrote:
> Why are you changing this here, as opposed to changing IntRange::forValueOfCanonicalType?
No particular reason. Initially i had trouble with moving it into `IntRange::forValueOfCanonicalType()`,
so i left that in the state that you saw. Now done.


================
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
----------------
Rakete1111 wrote:
> 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.
https://reviews.llvm.org/D38101?id=118143#inline-338796
That is exactly the flag to change the underlying type of an enum.
I believe the `RUN` lines are correct here.



Repository:
  rL LLVM

https://reviews.llvm.org/D39122