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