[PATCH] D39122: [Sema] Fixes for enum handling for tautological comparison diagnostics
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 21 08:22:47 PDT 2017
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
More information about the cfe-commits
mailing list