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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
