[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 llvm-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53
+  const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr());
+  if (!T)
+    return false;
----------------
JonasToth wrote:
> 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`?
I don't know?
I would guess no.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85
+template <typename LiteralType>
+llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix>
+UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix(
----------------
JonasToth wrote:
> 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.
Passing `LangOpts` turned out to be sufficient to move it into anon namespace.


================
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:
> > 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.
> *location*. which will still be a location, if only the first line of the warning is displayed.
> 
> We won't even win all that much.
> Sure, we will return `Optional<FixitHint>`, but we will still need to do all that internal stuff to find the old suffix, uppercase it, compare them, etc.
> 
> And we lose a very useful ability to check what it considered to be the old suffix in the tests.
> 
> It is basically the same question as in D51949.
> If you insist, sure, i can do that, but i *really* do believe this is **WRONG**.
And one more problem here, where do we point if we don't have a fix-it to get the suffix location from?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D52670