[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 13:45:34 PDT 2018

JonasToth added a comment.

> All those are UserDefinedLiteral, so we should be good.. https://godbolt.org/z/PcGi0B
>  Also, it seems the suffix can't be set for these constants: https://godbolt.org/z/YHTqke
>  So i'm not sure what to test. Can you give an example of a test?

I am not suggesting that these should be all uppercase or anything. But the tests should demonstrate, that user-defined literals do not impact analysis and give no false positives.

================
Comment at: clang-tidy/hicpp/HICPPTidyModule.cpp:24
 #include "../misc/StaticAssertCheck.h"
-#include "../bugprone/UndelegatedConstructorCheck.h"
 #include "../modernize/DeprecatedHeadersCheck.h"
----------------
spurious formatting fix, can be committed separate.

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

- `15 in decimal and is` (no comma)
- `skip to` instead of `skip until`

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:52
+AST_MATCHER(clang::IntegerLiteral, intHasSuffix) {
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
----------------
lebedev.ri wrote:
> 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_
fine then.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
+    return false;
----------------
lebedev.ri wrote:
> 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.
Ok. Can the `dyn_cast` be null at all? Shouldn't integerliteral always be a `BuiltinType`?

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85
+template <typename LiteralType>
+llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix>
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
----------------
lebedev.ri wrote:
> 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?
Why does it need to be a member? It looks like it only accesses `NewSuffix` which does nothing that requires private access to the check class.

================
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) ||
----------------
lebedev.ri wrote:
> 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..
Transformation is excluded of course.
The user can still silence the warning in case it is misplaced.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+  diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+      << LiteralType::Name << S.OldSuffix
----------------
lebedev.ri wrote:
> 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.
I don't understand that. The warning message does include the source location that would be clearly on the literal suffix and the warning without the suffix printed is clear as well. Having this slightly simpler diagnostic would simplify the code significantly.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:90
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
+    const clang::Expr &Literal, const clang::SourceManager &SM) {
+  const auto &L = cast<typename LiteralType::type>(Literal);
----------------
you can remove the `clang::` as we are in that namespace already.

================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:92
+  const auto &L = cast<typename LiteralType::type>(Literal);
+
+  NewSuffix ReplacementDsc;
----------------
I think you can drop some of the newlines in this function to make the code a bit more dense.

================
Comment at: clang-tidy/utils/ASTUtils.cpp:70

+// Check whether given IntegerLiteral is a synthetic value, like  __LINE__
+bool isSyntheticValue(const clang::SourceManager *SourceManager,
----------------
Please add the missing `.` at the end of the sentence

================
Comment at: docs/ReleaseNotes.rst:117
+  <clang-tidy/checks/cert-dcl16-c>` to :doc:`readability-uppercase-literal-suffix
+  <clang-tidy/checks/readability-uppercase-literal-suffix>`
+  added.
----------------
I believe the doc links for the aliases dangle right now. Please add the doc pages for them, too.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670