[PATCH] D124500: [clang-tidy] Support expressions of literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:32-33
+  // not a decimal floating-point literal
+  return std::none_of(
+      Begin, End, [](char C) { return C == '.' || std::toupper(C) == 'E'; });
+}
----------------
aaron.ballman wrote:
> Do we need to care about integer suffixes that make a non-integer type, like: https://godbolt.org/z/vx3xbGa41
I don't think those will be parsed as literal tokens by the preprocessor, but I'll check.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:99
+
+  if (!Current->isLiteral() || isStringLiteral(Current->getKind()) ||
+      !isIntegralConstant(*Current)) {
----------------
aaron.ballman wrote:
> I know this is code moved from elsewhere, but I suppose we never considered the odd edge case where a user does something like `"foo"[0]` as a really awful integer constant. :-D
It's always possible to write crazy contorted code and have a check not recognize it.  I don't think it's worthwhile to spend time trying to handle torture cases unless we can find data that shows it's prevalent in real world code.

If I was doing a code review and saw this:
```
enum {
    FOO = "foo"[0]
};
```
I'd flag that in a code review as bogus, whereas if I saw:
```
enum {
    FOO = 'f'
};
```
That would be acceptable, which is why character literals are accepted as an integral literal initializer for an enum in this check.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:185
+
+  if (Current->is(tok::TokenKind::question)) {
+    if (!advance())
----------------
aaron.ballman wrote:
> There is GNU extension in this space: https://godbolt.org/z/PrWY3T6hY
Do you have a link for the extension?


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:204
+  return true;
+}
+
----------------
aaron.ballman wrote:
> Comma operator?
Remember that the use case here is identifying expressions that are initializers for an enum.  If you were doing a code review and you saw this:
```
enum {
    FOO = (2, 3)
};
```
Would you be OK with that?  I wouldn't.  Clang even warns about it: https://godbolt.org/z/Y641cb8Y9

Therefore I deliberately left comma operator out of the grammar.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:26
+#define EXPR11 (1 + (2))
+#define EXPR12 ((1) + (2 + 0) + (1 * 1) + (1 / 1) + (1 | 1 ) + (1 & 1) + (1 << 1) + (1 >> 1) + (1 % 2) + (1 ^ 1))
+// CHECK-MESSAGES: :[[@LINE-12]]:1: warning: replace macro with enum [modernize-macro-to-enum]
----------------
aaron.ballman wrote:
> Other interesting tests I'd expect we could convert into an enum (at least theoretically):
> ```
> #define A 12 + +1
> #define B 12 - -1
> #define C (1, 2, 3)
> #define D 100 ? : 8
> #define E 100 ? 100 : 8
> #define F 'f'
> #define G "foo"[0]
> #define H 1 && 2
> #define I 1 || 2
> ```
Most of these (except comma operator and string subscript, see my comments earlier) are covered in the unit test for the matcher.  I'll add tests for these:

```
12 + +1
12 - -1
100 ? : 8
```


================
Comment at: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp:66
+    {false, "1.23"},
+    {false, "0x1p3"},
+    {false, R"("string")"},
----------------
aaron.ballman wrote:
> ```
> 12i
> .0
> ```
> 
`.0` is already covered by the case `1.23`.  I'm not home brewing tokenization, but using the Lexer to do that.

`12i` I need to investigate to find out what the Lexer does.


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

https://reviews.llvm.org/D124500