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