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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits