[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 accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from minor wording nits.



================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:31-32
+  static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("");
+  // Suffix can only consist of 'u' and 'l' chars, and can be a complex number.
+  // Also, in MS compatibility mode, suffixes like i32 are supported.
+  static constexpr llvm::StringLiteral Suffixes =
----------------
The comment is somewhat out of sync with the code because of the i and j suffixes as well. 


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:45
+  // C++17 introduced hexadecimal floating-point literals, and 'f' is both
+  // 15 in decimal and is 'f' as in 'floating point suffix'.
+  // So we can't just "skip to the chars that can be in the suffix".
----------------
I think you mean f is both a valid hexadecimal digit in a hex float literal and a valid floating-point literal suffix.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:101
+    return *NewSuffix;
+  // Nope, i guess we have to keep it as-is.
+  return llvm::None;
----------------
i -> I


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:212
+
+  // We won't *always* want to diagnose. We might have already-uppercase suffix.
+  if (auto Details = shouldReplaceLiteralSuffix<LiteralType>(
----------------
have already-uppercase suffix -> have a suffix that is already uppercase


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:216
+    auto Complaint = diag(Details->LiteralLocation,
+                          "%0 literal has suffix '%1', which is not upper-case")
+                     << LiteralType::Name << Details->OldSuffix;
----------------
upper-case -> uppercase


================
Comment at: docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst:28
+When the suffix is found, a case-insensitive lookup in that list is made,
+and if replacement is found, and it is different from the current suffix,
+only then the diagnostic is issued.
----------------
I'd reword this slightly:

"and if a replacement is found that is different from the current suffix, then the diagnostic is issued. This allows for fine-grained control of what suffixes to consider and what their replacements should be."


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670