[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 marked 8 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165
+
+  diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case")
+      << LiteralType::Name << S.OldSuffix
----------------
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**.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670