[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Two main points: I don't think this check is covering all of the suffixes (I don't see `q` or `i32` support, for instance), and at least for the CERT rule this is covering, it diagnoses more than it should. The CERT rule is specific to `l` vs `L` but imposes no requirements on `u` vs `U`.



================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:88-89
         "readability-string-compare");
+    CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
+        "readability-uppercase-literal-suffix");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
----------------
Please keep this sorted alphabetically.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:25
+
+struct IntegerLiteral {
+  using type = clang::IntegerLiteral;
----------------
It's unfortunate that clang::IntegerLiteral and clang::tidy::readability::IntegerLiteral are distinct types that are strongly related to one another. I'm not keen on this name, as it means reading the code, an unqualified IntegerLiteral is hard to reason about.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:30-31
+  static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("");
+  // Integer suffix can only consist of 'u' and 'l' chars.
+  static constexpr llvm::StringLiteral Suffixes = llvm::StringLiteral("uUlL");
+};
----------------
There are other suffixes for integer literals. See `NumericLiteralParser`.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:37
+
+struct FloatingLiteral {
+  using type = clang::FloatingLiteral;
----------------
Same naming concerns here.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:47-48
+  static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("pP");
+  // Floating suffix can only consist of 'u' and 'l' chars.
+  static constexpr llvm::StringLiteral Suffixes = llvm::StringLiteral("fFlL");
+};
----------------
There are more suffixes here (see `NumericLiteralParser` again), and this comment seems to be copy pasta.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:99-100
+                                                     const SourceManager &SM) {
+  auto Begin = GetMacroAwareLocation(Loc.getBegin(), SM);
+  auto End = GetMacroAwareLocation(Loc.getEnd(), SM);
+  if (!Begin || !End)
----------------
Don't use `auto` here.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:122
+  // The literal may have macro expansion, we need the final expanded src range.
+  auto Range = GetMacroAwareSourceRange(ReplacementDsc.LiteralLocation, SM);
+  if (!Range)
----------------
Don't use `auto` here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670