[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)

Sat Sep 29 03:43:07 PDT 2018

JonasToth added a comment.

Thanks for working on this!

Related as well: http://www.codingstandard.com/rule/4-2-1-ensure-that-the-u-suffix-is-applied-to-a-literal-used-in-a-context-requiring-an-unsigned-integral-expression/
I think its wort a alias is hicpp as well

Please add tests that use user-defined literals and ensure there are no collision and that they are not diagnosed. Some examples: https://en.cppreference.com/w/cpp/language/user_literal

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:26
+struct IntegerLiteral {
+  using ClangType = clang::IntegerLiteral;
+  static constexpr llvm::StringLiteral Name = llvm::StringLiteral("integer");
----------------
maybe `ClangType` is not a good name, how about just `type` to be consistent with e.g. std::vector convention.
The use-case in your template makes it clear, that we are talking about `LiteralType`s.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:33
+};
+constexpr llvm::StringLiteral IntegerLiteral::Name;
+constexpr llvm::StringLiteral IntegerLiteral::SkipFirst;
----------------
why are these declarations necessary?

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:42
+  // What should be skipped before looking for the Suffixes?
+  // Hexadecimal floating-point literals: skip until exponent first.
+  static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("pP");
----------------
The second line of the comment is slightly confusing, please make a full sentence out of it.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:52
+AST_MATCHER(clang::IntegerLiteral, intHasSuffix) {
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
----------------
as it hit me in my check: what about `(1)ul`? Is this syntactically correct and should be diagnosed too? (Please add tests if so).

In this case it should be `Note.getType().IgnoreParens().getTypePtr())`.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
+    return false;
----------------
Maybe the if could init `T`? It would require a second `return false;` if i am not mistaken, but looks more regular to me. No strong opinion from my side.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:69
+AST_MATCHER(clang::FloatingLiteral, fpHasSuffix) {
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
----------------
Same comment as above

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85
+template <typename LiteralType>
+llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix>
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
----------------
These types get really long. Is it possible to put `NewSuffix` into the anonymous namespace as well?

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:90
+
+  NewSuffix S;
+
----------------
GIven that this variable is used mutliple times in a longer function, i feel it shuold get a longer, more descriptive name

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:95
+  // Get the whole Integer Literal from the source buffer.
+  const StringRef LiteralSourceText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(S.Range), SM, getLangOpts());
----------------
Please check if the source text could be retrieved, with a final `bool` parameter, thats in/out and at least `assert` on that.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:106
+    Skip = LiteralSourceText.find_first_of(LiteralType::SkipFirst);
+    if (Skip == StringRef::npos) // We could be in non-hexadecimal fp literal.
+      Skip = 0;
----------------
please use `flloating-point` instead of `fp` (same in other places)

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:129
+  if (S.OldSuffix == S.NewSuffix)
+    return llvm::None; // The suffix was already fully uppercase.
+
----------------
Could this function return a `Optional<FixitHint>`? That would include the range and the relacement-text. I feel that is would simplify the code, especially the amount of state that one has to keep track of.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:135
+void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      stmt(integerLiteral(intHasSuffix())).bind(IntegerLiteral::Name), this);
----------------
I think you can merge this matcher to `stmt(eachOf(integerLiteral(intHasSuffix()).bind(), floatLiteral(fpHasSuffix()).bind()))`

`eachOf` because we want to match all, and not short-circuit.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:152
+  // Ignore literals that aren't fully written in the source code.
+  if (LiteralLocation.isMacroID() ||
+      Result.SourceManager->isMacroBodyExpansion(LiteralLocation) ||
----------------
Wouldn't it make sense to warn for the literals in macros?

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+  diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+      << LiteralType::Name << S.OldSuffix
----------------
Lets move this `diag` into the true branch of the if stmt and drop the ´else`.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+  diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+      << LiteralType::Name << S.OldSuffix
----------------
JonasToth wrote:
> Lets move this `diag` into the true branch of the if stmt and drop the ´else`.
I think the warning should point at the suffix, which can be retrieved from the replacement range. Then you don't need to include the suffix itself in the diagnostic

================
Comment at: docs/ReleaseNotes.rst:96

+- New alias :doc:`cert-dcl16-c
+  <clang-tidy/checks/cert-dcl16-c>` to :doc:`readability-uppercase-literal-suffix
----------------
lebedev.ri wrote:
> Eugene.Zelenko wrote:
> > Please move alias after new checks.
> BTW, is there some tool to actually add this alias? I didn't find it, and had to do it by hand..
So far there is nothing, but would be a nice addition :)

================
Comment at: docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst:9
+Detects when the integral literal or floating point (decimal or hexadecimal)
+literal has non-uppercase suffix, and suggests to make the suffix uppercase,
+with fix-it.
----------------
I feel that the sentence could be improved a bit.

- `literal has *a* non-uppercase`
- `suffix and provides a fix-it-hint with the uppercase suffix.` (i think the comma after `suffix` is not necessary)

================
Comment at: test/clang-tidy/readability-uppercase-literal-suffix-floating-point.cpp:26
+  static_assert(is_same<decltype(v0), const double>::value, "");
+  static_assert(v0 == 1, "");
+
----------------
nit: comparing int to double, same below.

================
Comment at: test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp:6
+
+template <class T, T v>
+struct integral_constant {
----------------
Please remove the TMP duplication and make a extra header for that, that is then included by these tests.

================
Comment at: test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp:31
+  static constexpr auto v1 = 0xfp0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: floating point literal suffix 'f' is not upper-case [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: static constexpr auto v1 = 0xfp0f;
----------------
you can safely drop the `[readability-..]` part of the warning message. It will just make problems if the check should be renamed at some point.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670