[PATCH] D125622: [clang-tidy] Reject invalid enum initializers in C files

[Aaron Ballman via Phabricator via cfe-commits]

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