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

[LegalizeAdulthood]

Summary

C requires that enum values fit into an int. Scan the macro tokens
present in an initializing expression and reject macros that contain
tokens that have suffixes making them larger than int.

C forbids the comma operator in enum initializing expressions, so
optionally reject comma operator.

Fixes #55467

[aaron.ballman]

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.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

324–331

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.

326

And C89?

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

2

It'd be useful to run this test in C++ mode as well to demonstrate the behavioral differences.

4–8

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?

10

It's also forbidden in C++.

11

Can you add a test:

#define GOOD_OP (1, 2)

to make sure it still gets converted to an enum?

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

10

C++ just talks about a constant expression and doesn't explicitly disallow operator, in such expressions that I could find. Can you point me to where it is disallowed?

If it is disallowed universally, then I don't see why you asked me to parse it :)

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

10

C++ just talks about a constant expression and doesn't explicitly disallow operator, in such expressions that I could find. Can you point me to where it is disallowed?

It falls out from the grammar:

http://eel.is/c++draft/enum#nt:enumerator-definition
http://eel.is/c++draft/expr.const#nt:constant-expression
http://eel.is/c++draft/expr.cond#nt:conditional-expression

If it is disallowed universally, then I don't see why you asked me to parse it :)

It can show up in paren expressions and the interface is generally about integer literal expressions.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

10

OK, yeah, I was looking at the PDF and it doesn't have those nice navigable links, I'll have to keep that site in mind.

So.... we have to disallow top-level comma but allow it as a parenthesized expression. Tricky!

11

Yeah, another one I was thinking of is when someone does something as disgusting as this.

#define ARGS 1, 2
#define OTHER_ARGS (1, 2)

int f(int x, int y) { return x + y; }

int main()
{
    return f(ARGS) + f OTHER_ARGS;
}

However, the only real way to handle avoiding conversion of the 2nd case is to examine the context of macro expansion. This is another edge case that will have to be handled subsequently.

This gets tricky because AFAIK there is no way to select expressions in the AST that result from macro expansion. You have to match the macro expansion locations against AST nodes to identify the node(s) that match the expansion location yourself.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

11

That's a good test case (for some definition of good)! :-)

At this point, there's a few options:

1. Go back to the way things were -- disallow comma expressions, even in parens. Document it as a limitation.
2. Accept comma expressions in parens, ignore the problem case like you described above. Document it as a limitation?
3. Try to solve every awful code construct we can ever imagine. Document the check is perfect in every way, but probably not land it for several years.

I think I'd be happy with #2 but I could also live with #1

[LegalizeAdulthood]

• Update from review comments
• Disallow operator, unless inside parentheses

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

324–331

The most recent diff handles the issue with operator, but the previous version was only handling the overly-big literal

326

C99 was the earliest dialect of C I could find in LangOptions.def. If you can show me an earlier version for C, I'm happy to switch it.

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

4–8

I was trying to find a way to make it fail on gcc/clang on compiler explorer and I couldn't get it to fail in either compiler....

Where is the extension documented? It appears to be on by default in both compilers.

11

In the most recent diff, I'm supporting operator, inside parens for C++ and never allowing it in C.

I plan to do a subsequent enhancement that examines the expansion locations of macros and rejects any macro whose expansion doesn't encompass a single expression. In other words, if the tokens in the macro somehow get incorporated into some syntactic element that isn't completely encased in a single expression, then reject the macro.

[LegalizeAdulthood]

• Add C++ test case for acceptable operator, initializer

[aaron.ballman]

LGTM with a few comments that can be addressed when you commit.

clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp

87–89

91–93

I (personally) think this is a more clear form of initialization to most folks, but don't insist.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

326

This keeps catching folks out, I may add a helper method to make this far more clear. !LangOpts.CPlusPlus means "in a C mode" for us currently.

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

4–8

I don't think we have any documentation for it (we basically don't bother to document our implementation-defined behavior, which drives me mildly nuts -- we fall back on "the source code is the definitive documentation", which is not user-friendly at all).

That's why I was wondering if we wanted a pedantic vs non-pedantic option here. Pedantically, the behavior you have today is correct. However, because this is a super common extension to compilers, we might want to allow it to be transformed despite being pedantically an extension.

We can handle that as a follow-up though.

[LegalizeAdulthood]

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

4–8

Is there a way to force it to fail with some compiler flag? If you could show me on compiler explorer, that would be great.

[LegalizeAdulthood]

Update from review comments

clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.h

 
 #include <clang/Lex/Token.h>
 #include <llvm/ADT/ArrayRef.h>
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+enum class LiteralSize {
+  Unknown = 0,
+  Int,
+  UnsignedInt,
+  Long,
+  UnsignedLong,
+  LongLong,
+  UnsignedLongLong
+};
+
 // 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++.
+// precedence rules of C++.  Optionally exclude comma operator expressions.
 class IntegralLiteralExpressionMatcher {
 public:
-  IntegralLiteralExpressionMatcher(ArrayRef<Token> Tokens)
-      : Current(Tokens.begin()), End(Tokens.end()) {}
+  IntegralLiteralExpressionMatcher(ArrayRef<Token> Tokens, bool CommaAllowed)
+      : Current(Tokens.begin()), End(Tokens.end()), CommaAllowed(CommaAllowed) {
+  }
 
   bool match();
+  LiteralSize largestLiteralSize() const;
 
 private:
   bool advance();
   bool consume(tok::TokenKind Kind);
   bool nonTerminalChainedExpr(
       bool (IntegralLiteralExpressionMatcher::*NonTerminal)(),
       const std::function<bool(Token)> &IsKind);
   template <tok::TokenKind Kind>
private:
   bool logicalAndExpr();
   bool logicalOrExpr();
   bool conditionalExpr();
   bool commaExpr();
   bool expr();
 
   ArrayRef<Token>::iterator Current;
   ArrayRef<Token>::iterator End;
+  LiteralSize LargestSize{LiteralSize::Unknown};
+  bool CommaAllowed;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif

clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp

 //===--- IntegralLiteralExpressionMatcher.cpp - clang-tidy ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "IntegralLiteralExpressionMatcher.h"
 
+#include <algorithm>
 #include <cctype>
 #include <stdexcept>
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 // Validate that this literal token is a valid integer literal.  A literal token
bool IntegralLiteralExpressionMatcher::unaryOperator() {
   if (Current->isOneOf(tok::TokenKind::minus, tok::TokenKind::plus,
                        tok::TokenKind::tilde, tok::TokenKind::exclaim)) {
     return advance();
   }
 
   return true;
 }
 
+static LiteralSize literalTokenSize(const Token &Tok) {
+  unsigned int Length = Tok.getLength();
+  if (Length <= 1)
+    return LiteralSize::Int;
+
+  bool SeenUnsigned = false;
+  bool SeenLong = false;
+  bool SeenLongLong = false;
+  const char *Text = Tok.getLiteralData();
+  for (unsigned int End = Length - 1; End > 0; --End) {
+    if (std::isdigit(Text[End]))
+      break;
+
+    if (std::toupper(Text[End]) == 'U')
+      SeenUnsigned = true;
+    else if (std::toupper(Text[End]) == 'L') {
+      if (SeenLong)
+        SeenLongLong = true;
+      SeenLong = true;
+    }
+  }
+
+  if (SeenLongLong) {
+    if (SeenUnsigned)
+      return LiteralSize::UnsignedLongLong;
+
+    return LiteralSize::LongLong;
+  }
+  if (SeenLong) {
+    if (SeenUnsigned)
+      return LiteralSize::UnsignedLong;
+
+    return LiteralSize::Long;
+  }
+  if (SeenUnsigned)
+    return LiteralSize::UnsignedInt;
+
+  return LiteralSize::Int;
+}
+
+static bool operator<(LiteralSize LHS, LiteralSize RHS) {
+  return static_cast<int>(LHS) < static_cast<int>(RHS);
+}
+
 bool IntegralLiteralExpressionMatcher::unaryExpr() {
   if (!unaryOperator())
     return false;
 
   if (consume(tok::TokenKind::l_paren)) {
     if (Current == End)
       return false;
 
     if (!expr())
       return false;
 
     if (Current == End)
       return false;
 
     return consume(tok::TokenKind::r_paren);
   }
 
   if (!Current->isLiteral() || isStringLiteral(Current->getKind()) ||
       !isIntegralConstant(*Current)) {
     return false;
   }
+
+  LargestSize = std::max(LargestSize, literalTokenSize(*Current));
   ++Current;
+
   return true;
 }
 
 bool IntegralLiteralExpressionMatcher::multiplicativeExpr() {
   return nonTerminalChainedExpr<tok::TokenKind::star, tok::TokenKind::slash,
                                 tok::TokenKind::percent>(
       &IntegralLiteralExpressionMatcher::unaryExpr);
 }
if (Current->is(tok::TokenKind::question)) {
 
     if (!expr())
       return false;
   }
   return true;
 }
 
 bool IntegralLiteralExpressionMatcher::commaExpr() {
-  return nonTerminalChainedExpr<tok::TokenKind::comma>(
-      &IntegralLiteralExpressionMatcher::conditionalExpr);
+  auto Pred = CommaAllowed
+                  ? std::function<bool(Token)>(
+                        [](Token Tok) { return Tok.is(tok::TokenKind::comma); })
+                  : std::function<bool(Token)>([](Token) { return false; });
+  return nonTerminalChainedExpr(
+      &IntegralLiteralExpressionMatcher::conditionalExpr, Pred);
 }
 
 bool IntegralLiteralExpressionMatcher::expr() { return commaExpr(); }
 
 bool IntegralLiteralExpressionMatcher::match() {
-  return expr() && Current == End;
+  // Top-level allowed expression is conditionalExpr(), not expr(), because
+  // comma operators are only valid initializers when used inside parentheses.
+  return conditionalExpr() && Current == End;
+}
+
+LiteralSize IntegralLiteralExpressionMatcher::largestLiteralSize() const {
+  return LargestSize;
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

if (Reason == EnterFile) {
     assert(CurrentFile->ConditionScopes == 0);
     Files.pop_back();
   }
   CurrentFile = &Files.back();
 }
 
 bool MacroToEnumCallbacks::isInitializer(ArrayRef<Token> MacroTokens)
 {
-  IntegralLiteralExpressionMatcher Matcher(MacroTokens);
-  return Matcher.match();
+  IntegralLiteralExpressionMatcher Matcher(MacroTokens, LangOpts.C99 == 0);
+  bool Matched = Matcher.match();
+  bool isC = !LangOpts.CPlusPlus;
+  if (isC && (Matcher.largestLiteralSize() != LiteralSize::Int &&
+              Matcher.largestLiteralSize() != LiteralSize::UnsignedInt))
+    return false;
+
+  return Matched;
 }
 
 
 // Any defined but rejected macro is scanned for identifiers that
 // are to be excluded as enums.
 void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok,
                                         const MacroDirective *MD) {
   // Include guards are never candidates for becoming an enum.

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c

+// RUN: %check_clang_tidy %s modernize-macro-to-enum %t
+
+// 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
+
+// C forbids comma operator in initializing expressions.
+#define BAD_OP 1, 2
+
+#define SIZE_OK1 1
+#define SIZE_OK2 1U
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum [modernize-macro-to-enum]
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'SIZE_OK1' defines an integral constant; prefer an enum instead
+// CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'SIZE_OK2' defines an integral constant; prefer an enum instead

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

 // CHECK-MESSAGES: :[[@LINE-2]]:1: warning: replace macro with enum
 // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN5' defines an integral constant; prefer an enum instead
 // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: macro 'COMPLEX_PAREN6' defines an integral constant; prefer an enum instead
 // CHECK-FIXES: enum {
 // CHECK-FIXES-NEXT: COMPLEX_PAREN5 = (+1),
 // CHECK-FIXES-NEXT: COMPLEX_PAREN6 = ((+1))
 // CHECK-FIXES-NEXT: };
 
+#define GOOD_COMMA (1, 2)
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: replace macro with enum
+// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: macro 'GOOD_COMMA' defines an integral constant; prefer an enum instead
+// CHECK-FIXES: enum {
+// CHECK-FIXES-NEXT: GOOD_COMMA = (1, 2)
+// CHECK-FIXES-NEXT: };
+
 // Macros appearing in conditional expressions can't be replaced
 // by enums.
 #define USE_FOO 1
 #define USE_BAR 0
 #define USE_IF 1
 #define USE_ELIF 1
 #define USE_IFDEF 1
 #define USE_IFNDEF 1

 
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
 #define EPS1 1.0F
 #define EPS2 1e5
 #define EPS3 1.
 
+// Ignore macros invoking comma operator unless they are inside parens.
+#define BAD_COMMA 1, 2
+
 extern void draw(unsigned int Color);
 
 void f()
 {
   // Usage of macros converted to enums should still compile.
   draw(RED);
   draw(GREEN | RED);
   draw(BLUE + RED);

clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

while (!End) {
     Token Tok;
     End = Lex.LexFromRawLexer(Tok);
     Tokens.push_back(Tok);
   }
 
   return Tokens;
 }
 
-static bool matchText(const char *Text) {
+static bool matchText(const char *Text, bool AllowComma) {
   std::vector<Token> Tokens{tokenify(Text)};
-  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens);
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, AllowComma);
 
   return Matcher.match();
 }
 
+static modernize::LiteralSize sizeText(const char *Text) {
+  std::vector<Token> Tokens{tokenify(Text)};
+  modernize::IntegralLiteralExpressionMatcher Matcher(Tokens, true);
+  if (Matcher.match())
+    return Matcher.largestLiteralSize();
+  return modernize::LiteralSize::Unknown;
+}
+
+static const char *toString(modernize::LiteralSize Value) {
+  switch (Value) {
+  case modernize::LiteralSize::Int:
+    return "Int";
+  case modernize::LiteralSize::UnsignedInt:
+    return "UnsignedInt";
+  case modernize::LiteralSize::Long:
+    return "Long";
+  case modernize::LiteralSize::UnsignedLong:
+    return "UnsignedLong";
+  case modernize::LiteralSize::LongLong:
+    return "LongLong";
+  case modernize::LiteralSize::UnsignedLongLong:
+    return "UnsignedLongLong";
+  default:
+    return "Unknown";
+  }
+}
+
 namespace {
 
-struct Param {
+struct MatchParam {
+  bool AllowComma;
   bool Matched;
   const char *Text;
 
-  friend std::ostream &operator<<(std::ostream &Str, const Param &Value) {
-    return Str << "Matched: " << std::boolalpha << Value.Matched << ", Text: '"
-               << Value.Text << "'";
+  friend std::ostream &operator<<(std::ostream &Str, const MatchParam &Value) {
+    return Str << "Allow operator,: " << std::boolalpha << Value.AllowComma
+               << ", Matched: " << std::boolalpha << Value.Matched
+               << ", Text: '" << Value.Text << '\'';
+  }
+};
+
+struct SizeParam {
+  modernize::LiteralSize Size;
+  const char *Text;
+
+  friend std::ostream &operator<<(std::ostream &Str, const SizeParam &Value) {
+    return Str << "Size: " << toString(Value.Size) << ", Text: '" << Value.Text << '\'';
   }
 };
 
-class MatcherTest : public ::testing::TestWithParam<Param> {};
+class MatcherTest : public ::testing::TestWithParam<MatchParam> {};
+
+class SizeTest : public ::testing::TestWithParam<SizeParam> {};
 
 } // namespace
 
-static const Param Params[] = {
+static const MatchParam MatchParams[] = {
     // Accept integral literals.
-    {true, "1"},
-    {true, "0177"},
-    {true, "0xdeadbeef"},
-    {true, "0b1011"},
-    {true, "'c'"},
+    {true, true, "1"},
+    {true, true, "0177"},
+    {true, true, "0xdeadbeef"},
+    {true, true, "0b1011"},
+    {true, true, "'c'"},
     // Reject non-integral literals.
-    {false, "1.23"},
-    {false, "0x1p3"},
-    {false, R"("string")"},
-    {false, "1i"},
+    {true, false, "1.23"},
+    {true, false, "0x1p3"},
+    {true, false, R"("string")"},
+    {true, false, "1i"},
 
     // Accept literals with these unary operators.
-    {true, "-1"},
-    {true, "+1"},
-    {true, "~1"},
-    {true, "!1"},
+    {true, true, "-1"},
+    {true, true, "+1"},
+    {true, true, "~1"},
+    {true, true, "!1"},
     // Reject invalid unary operators.
-    {false, "1-"},
-    {false, "1+"},
-    {false, "1~"},
-    {false, "1!"},
+    {true, false, "1-"},
+    {true, false, "1+"},
+    {true, false, "1~"},
+    {true, false, "1!"},
 
     // Accept valid binary operators.
-    {true, "1+1"},
-    {true, "1-1"},
-    {true, "1*1"},
-    {true, "1/1"},
-    {true, "1%2"},
-    {true, "1<<1"},
-    {true, "1>>1"},
-    {true, "1<=>1"},
-    {true, "1<1"},
-    {true, "1>1"},
-    {true, "1<=1"},
-    {true, "1>=1"},
-    {true, "1==1"},
-    {true, "1!=1"},
-    {true, "1&1"},
-    {true, "1^1"},
-    {true, "1|1"},
-    {true, "1&&1"},
-    {true, "1||1"},
-    {true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
-    {true, "1- -1"},
-    {true, "1,1"},
+    {true, true, "1+1"},
+    {true, true, "1-1"},
+    {true, true, "1*1"},
+    {true, true, "1/1"},
+    {true, true, "1%2"},
+    {true, true, "1<<1"},
+    {true, true, "1>>1"},
+    {true, true, "1<=>1"},
+    {true, true, "1<1"},
+    {true, true, "1>1"},
+    {true, true, "1<=1"},
+    {true, true, "1>=1"},
+    {true, true, "1==1"},
+    {true, true, "1!=1"},
+    {true, true, "1&1"},
+    {true, true, "1^1"},
+    {true, true, "1|1"},
+    {true, true, "1&&1"},
+    {true, true, "1||1"},
+    {true, true, "1+ +1"}, // A space is needed to avoid being tokenized as ++ or --.
+    {true, true, "1- -1"},
+    // Comma is only valid when inside parentheses.
+    {true, true, "(1,1)"},
     // Reject invalid binary operators.
-    {false, "1+"},
-    {false, "1-"},
-    {false, "1*"},
-    {false, "1/"},
-    {false, "1%"},
-    {false, "1<<"},
-    {false, "1>>"},
-    {false, "1<=>"},
-    {false, "1<"},
-    {false, "1>"},
-    {false, "1<="},
-    {false, "1>="},
-    {false, "1=="},
-    {false, "1!="},
-    {false, "1&"},
-    {false, "1^"},
-    {false, "1|"},
-    {false, "1&&"},
-    {false, "1||"},
-    {false, "1,"},
-    {false, ",1"},
+    {true, false, "1+"},
+    {true, false, "1-"},
+    {true, false, "1*"},
+    {true, false, "1/"},
+    {true, false, "1%"},
+    {true, false, "1<<"},
+    {true, false, "1>>"},
+    {true, false, "1<=>"},
+    {true, false, "1<"},
+    {true, false, "1>"},
+    {true, false, "1<="},
+    {true, false, "1>="},
+    {true, false, "1=="},
+    {true, false, "1!="},
+    {true, false, "1&"},
+    {true, false, "1^"},
+    {true, false, "1|"},
+    {true, false, "1&&"},
+    {true, false, "1||"},
+    {true, false, "1,"},
+    {true, false, ",1"},
+    {true, false, "1,1"},
 
     // Accept valid ternary operators.
-    {true, "1?1:1"},
-    {true, "1?:1"}, // A gcc extension treats x ? : y as x ? x : y.
+    {true, true, "1?1:1"},
+    {true, true, "1?:1"}, // A gcc extension treats x ? : y as x ? x : y.
     // Reject invalid ternary operators.
-    {false, "?"},
-    {false, "?1"},
-    {false, "?:"},
-    {false, "?:1"},
-    {false, "?1:"},
-    {false, "?1:1"},
-    {false, "1?"},
-    {false, "1?1"},
-    {false, "1?:"},
-    {false, "1?1:"},
+    {true, false, "?"},
+    {true, false, "?1"},
+    {true, false, "?:"},
+    {true, false, "?:1"},
+    {true, false, "?1:"},
+    {true, false, "?1:1"},
+    {true, false, "1?"},
+    {true, false, "1?1"},
+    {true, false, "1?:"},
+    {true, false, "1?1:"},
 
     // Accept parenthesized expressions.
-    {true, "(1)"},
-    {true, "((+1))"},
-    {true, "((+(1)))"},
-    {true, "(-1)"},
-    {true, "-(1)"},
-    {true, "(+1)"},
-    {true, "((+1))"},
-    {true, "+(1)"},
-    {true, "(~1)"},
-    {true, "~(1)"},
-    {true, "(!1)"},
-    {true, "!(1)"},
-    {true, "(1+1)"},
-    {true, "(1-1)"},
-    {true, "(1*1)"},
-    {true, "(1/1)"},
-    {true, "(1%2)"},
-    {true, "(1<<1)"},
-    {true, "(1>>1)"},
-    {true, "(1<=>1)"},
-    {true, "(1<1)"},
-    {true, "(1>1)"},
-    {true, "(1<=1)"},
-    {true, "(1>=1)"},
-    {true, "(1==1)"},
-    {true, "(1!=1)"},
-    {true, "(1&1)"},
-    {true, "(1^1)"},
-    {true, "(1|1)"},
-    {true, "(1&&1)"},
-    {true, "(1||1)"},
-    {true, "(1?1:1)"},
+    {true, true, "(1)"},
+    {true, true, "((+1))"},
+    {true, true, "((+(1)))"},
+    {true, true, "(-1)"},
+    {true, true, "-(1)"},
+    {true, true, "(+1)"},
+    {true, true, "((+1))"},
+    {true, true, "+(1)"},
+    {true, true, "(~1)"},
+    {true, true, "~(1)"},
+    {true, true, "(!1)"},
+    {true, true, "!(1)"},
+    {true, true, "(1+1)"},
+    {true, true, "(1-1)"},
+    {true, true, "(1*1)"},
+    {true, true, "(1/1)"},
+    {true, true, "(1%2)"},
+    {true, true, "(1<<1)"},
+    {true, true, "(1>>1)"},
+    {true, true, "(1<=>1)"},
+    {true, true, "(1<1)"},
+    {true, true, "(1>1)"},
+    {true, true, "(1<=1)"},
+    {true, true, "(1>=1)"},
+    {true, true, "(1==1)"},
+    {true, true, "(1!=1)"},
+    {true, true, "(1&1)"},
+    {true, true, "(1^1)"},
+    {true, true, "(1|1)"},
+    {true, true, "(1&&1)"},
+    {true, true, "(1||1)"},
+    {true, true, "(1?1:1)"},
 
     // Accept more complicated "chained" expressions.
-    {true, "1+1+1"},
-    {true, "1+1+1+1"},
-    {true, "1+1+1+1+1"},
-    {true, "1*1*1"},
-    {true, "1*1*1*1"},
-    {true, "1*1*1*1*1"},
-    {true, "1<<1<<1"},
-    {true, "4U>>1>>1"},
-    {true, "1<1<1"},
-    {true, "1>1>1"},
-    {true, "1<=1<=1"},
-    {true, "1>=1>=1"},
-    {true, "1==1==1"},
-    {true, "1!=1!=1"},
-    {true, "1&1&1"},
-    {true, "1^1^1"},
-    {true, "1|1|1"},
-    {true, "1&&1&&1"},
-    {true, "1||1||1"},
-    {true, "1,1,1"},
+    {true, true, "1+1+1"},
+    {true, true, "1+1+1+1"},
+    {true, true, "1+1+1+1+1"},
+    {true, true, "1*1*1"},
+    {true, true, "1*1*1*1"},
+    {true, true, "1*1*1*1*1"},
+    {true, true, "1<<1<<1"},
+    {true, true, "4U>>1>>1"},
+    {true, true, "1<1<1"},
+    {true, true, "1>1>1"},
+    {true, true, "1<=1<=1"},
+    {true, true, "1>=1>=1"},
+    {true, true, "1==1==1"},
+    {true, true, "1!=1!=1"},
+    {true, true, "1&1&1"},
+    {true, true, "1^1^1"},
+    {true, true, "1|1|1"},
+    {true, true, "1&&1&&1"},
+    {true, true, "1||1||1"},
+    {true, true, "(1,1,1)"},
+
+    // Optionally reject comma operator
+    {false, false, "1,1"}
 };
 
 TEST_P(MatcherTest, MatchResult) {
-  EXPECT_TRUE(matchText(GetParam().Text) == GetParam().Matched);
+  const MatchParam &Param = GetParam();
+ 
+  EXPECT_TRUE(matchText(Param.Text, Param.AllowComma) == Param.Matched);
 }
 
-INSTANTIATE_TEST_SUITE_P(TokenExpressionParserTests, MatcherTest,
-                         ::testing::ValuesIn(Params));
+INSTANTIATE_TEST_SUITE_P(IntegralLiteralExpressionMatcherTests, MatcherTest,
+                         ::testing::ValuesIn(MatchParams));
+
+static const SizeParam SizeParams[] = {
+    {modernize::LiteralSize::Int, "1"},
+    {modernize::LiteralSize::UnsignedInt, "1U"},
+    {modernize::LiteralSize::Long, "1L"},
+    {modernize::LiteralSize::UnsignedLong, "1UL"},
+    {modernize::LiteralSize::UnsignedLong, "1LU"},
+    {modernize::LiteralSize::LongLong, "1LL"},
+    {modernize::LiteralSize::UnsignedLongLong, "1ULL"},
+    {modernize::LiteralSize::UnsignedLongLong, "1LLU"}};
+
+TEST_P(SizeTest, TokenSize) {
+  EXPECT_EQ(sizeText(GetParam().Text), GetParam().Size);
+};
+
+INSTANTIATE_TEST_SUITE_P(IntegralLiteralExpressionMatcherTests, SizeTest,
+                         ::testing::ValuesIn(SizeParams));
 
 } // namespace test
 } // namespace tidy
 } // namespace clang