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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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();
----------------
lebedev.ri wrote:
> 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.
Consistency is best, so you might as well leave it. A follow-up NFC commit would be fine if you wanted to switch everything to `auto`.


================
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 };
 };
----------------
lebedev.ri wrote:
> 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.
Okay, that comment adds sufficient clarity for me.


================
Comment at: lib/Sema/SemaChecking.cpp:8593
+  Min = 1U << 1U,  // e.g. -32768 for short
+  Both = Max | Min // when the value is both the Min and the Max limit at the
+                   // same time; e.g. in C++, A::a in enum A { a = 0 };
----------------
when -> When


================
Comment at: lib/Sema/SemaChecking.cpp:8619
+  if (OtherRange.Width == 0)
+    return Value == 0 ? LimitType::Both : llvm::Optional<LimitType>();
+
----------------
Instead of default constructing the Optional, you should use `llvm::None` instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D39122