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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:33
+};
+constexpr llvm::StringLiteral IntegerLiteral::Name;
+constexpr llvm::StringLiteral IntegerLiteral::SkipFirst;
----------------
JonasToth wrote:
> why are these declarations necessary?
Well, because these are *declarations*.
Else the linker complains, since i'm using these in non-`constexpr` context, but only provided definitions.


================
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");
----------------
JonasToth wrote:
> The second line of the comment is slightly confusing, please make a full sentence out of it.
Rewrote, hopefully better?


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:52
+AST_MATCHER(clang::IntegerLiteral, intHasSuffix) {
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
----------------
JonasToth wrote:
> 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())`.
clang says invalid
https://godbolt.org/z/R8bGe_


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
+    return false;
----------------
JonasToth wrote:
> 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.
Then we will not have an early-return, which is worse than this.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85
+template <typename LiteralType>
+llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix>
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
----------------
JonasToth wrote:
> These types get really long. Is it possible to put `NewSuffix` into the anonymous namespace as well?
No, because `shouldReplaceLiteralSuffix()` is a member function which returns that type.
I think it should stay a member function, so theoretically `NewSuffix` could be a [second] template param, but that is kinda ugly..
I also can't simplify it via `using` because the `NewSuffix` is private.

Perhaps we should keep this as-is?


================
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());
----------------
JonasToth wrote:
> Please check if the source text could be retrieved, with a final `bool` parameter, thats in/out and at least `assert` on that.
I looked at a randomly-selected few dozen calls to this function within clang-tidy, and none of those do this.
But `assert` added, better than nothing.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:129
+  if (S.OldSuffix == S.NewSuffix)
+    return llvm::None; // The suffix was already fully uppercase.
+
----------------
JonasToth wrote:
> 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.
I still want to see the old suffix, but i think we can form the FixitHint here, and store it.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:135
+void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      stmt(integerLiteral(intHasSuffix())).bind(IntegerLiteral::Name), this);
----------------
JonasToth wrote:
> 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.
`bind()` still wants the name though.


================
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) ||
----------------
JonasToth wrote:
> Wouldn't it make sense to warn for the literals in macros?
Very obscure example (like all macros is), but i think it shows the point:
```
#include <stdio.h>
#include <stdio.h>

#define xstr(s) str(s)
#define str(s) #s

#define dump(X) printf("%u is " str(X) "nits", X);

int main () {
  dump(1u);

  return 0;
}
```
will normally print `1 is 1units`
But if you uppercase it, it will print `1 is 1Units`.

I would honestly prefer to give macros a pass here..


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+  diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+      << LiteralType::Name << S.OldSuffix
----------------
JonasToth wrote:
> 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
If the warnings are aggregated (i.e. not raw `make` dump), with only the first line shown, the suffix will still be invisible.

Regarding the pointer direction, i'm not sure.
For now i have reworded the diag to justify pointing at the literal itself.


================
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.
----------------
JonasToth wrote:
> 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)
Words are the hardest part :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670