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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:25
+
+  // not a hexadecimal floating-point literal
+  if (Token.getLength() > 2 && Begin[0] == '0' && std::toupper(Begin[1]) == 'X')
----------------
(Same suggestion elsewhere -- just double check that all the comments are full sentences with capitalization and punctuation.)


================
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'; });
+}
----------------
Do we need to care about integer suffixes that make a non-integer type, like: https://godbolt.org/z/vx3xbGa41


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:99
+
+  if (!Current->isLiteral() || isStringLiteral(Current->getKind()) ||
+      !isIntegralConstant(*Current)) {
----------------
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


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:185
+
+  if (Current->is(tok::TokenKind::question)) {
+    if (!advance())
----------------
There is GNU extension in this space: https://godbolt.org/z/PrWY3T6hY


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:204
+  return true;
+}
+
----------------
Comma operator?


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h:9
+
+#pragma once
+
----------------
We don't use #pragma once (not portable, not reliable).


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h:18-20
+// Parses an array of tokens and returns true if they conform to the rules of
+// C++ for whole expressions involving integral literals.  Follows the operator
+// precedence rules of C++.
----------------
Oh boy, I'm not super excited about having another parser to maintain...

It'd be nice if we had a ParserUtils.cpp/h file that made it easier to go from an arbitrary array of tokens to AST nodes + success/failure information on parsing the tokens. It's not strictly needed for what you're trying to accomplish here, but it would be a much more general interface and it would remove the support burden from adding another parser that's out of Clang's tree.


================
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]
----------------
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
```


================
Comment at: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp:66
+    {false, "1.23"},
+    {false, "0x1p3"},
+    {false, R"("string")"},
----------------
```
12i
.0
```



================
Comment at: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp:99
+    {true, "1&&1"},
+    {true, "1||1"},
+    // invalid binary operator
----------------
```
100 ? : 10
1, 2
```


================
Comment at: clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp:134
+    {false, "1?1:"},
+    {false, "1?:1"},
+
----------------
This one is valid


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

https://reviews.llvm.org/D124500