aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM with a few comments that can be addressed when you commit. ================ Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:87-89 + if (Length <= 1) { + return LiteralSize::Int; + } ---------------- ================ Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:91-93 + bool SeenUnsigned{}; + bool SeenLong{}; + bool SeenLongLong{}; ---------------- I (personally) think this is a more clear form of initialization to most folks, but don't insist. ================ Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:326 + bool Matched = Matcher.match(); + if (LangOpts.C99 && + (Matcher.largestLiteralSize() != LiteralSize::Int && ---------------- LegalizeAdulthood wrote: > aaron.ballman wrote: > > And C89? > `C99` was the earliest dialect of C I could find in `LangOptions.def`. If > you can show me an earlier version for C, I'm happy to switch it. This keeps catching folks out, I may add a helper method to make this far more clear. `!LangOpts.CPlusPlus` means "in a C mode" for us currently. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:3-7 +// C requires enum values to fit into an int. +#define TOO_BIG1 1L +#define TOO_BIG2 1UL +#define TOO_BIG3 1LL +#define TOO_BIG4 1ULL ---------------- LegalizeAdulthood wrote: > aaron.ballman wrote: > > How do we want to handle the fact that Clang has an extension to allow > > this? Perhaps we want a config option for pedantic vs non-pedantic fixes? > I was trying to find a way to make it fail on gcc/clang on compiler explorer > and I couldn't get it to fail in either compiler.... > > Where is the extension documented? It appears to be on by default in both > compilers. I don't think we have any documentation for it (we basically don't bother to document our implementation-defined behavior, which drives me mildly nuts -- we fall back on "the source code is the definitive documentation", which is not user-friendly at all). That's why I was wondering if we wanted a pedantic vs non-pedantic option here. Pedantically, the behavior you have today is correct. However, because this is a super common extension to compilers, we might want to allow it to be transformed despite being pedantically an extension. We can handle that as a follow-up though. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125622/new/ https://reviews.llvm.org/D125622 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits