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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Note, I'm in C standards committee meetings all week this week, so I expect I won't be doing many reviews this week and I'll be catching up as best I can starting next week.



================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:324
 {
-  IntegralLiteralExpressionMatcher Matcher(MacroTokens);
-  return Matcher.match();
+  IntegralLiteralExpressionMatcher Matcher(MacroTokens, LangOpts.C99 == 0);
+  bool Matched = Matcher.match();
----------------
Huh? Comma wasn't allowed in C89 either... In fact, there's no language mode which allows top-level commas in either C or C++ -- the issue with the comma operator is a parsing one. You can't tell the difference between the comma being part of the initializer expression or the comma being the separator between enumerators.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:326
+  bool Matched = Matcher.match();
+  if (LangOpts.C99 &&
+      (Matcher.largestLiteralSize() != LiteralSize::Int &&
----------------
And C89?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:1
+// RUN: %check_clang_tidy %s modernize-macro-to-enum %t
+
----------------
It'd be useful to run this test in C++ mode as well to demonstrate the behavioral differences.


================
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
----------------
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?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:9
+
+// C forbids comma operator in initializing expressions.
+#define BAD_OP 1, 2
----------------
It's also forbidden in C++.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:10
+// C forbids comma operator in initializing expressions.
+#define BAD_OP 1, 2
+
----------------
Can you add a test:
```
#define GOOD_OP (1, 2)
```
to make sure it still gets converted to an enum?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125622/new/

https://reviews.llvm.org/D125622