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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth 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
----------------
lebedev.ri wrote:
> 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?
That point is valid, but if we dont have a suffix location, there is no suffix?
I dont insist :)


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:117
+
+  // So was this literal fully spelled,
+  // or is it a product of macro expansion and/or concatenation?
----------------
the comma is not necessary


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:119
+  // or is it a product of macro expansion and/or concatenation?
+  const bool RangeCanBeFixed =
+      utils::rangeCanBeFixed(ReplacementDsc.LiteralLocation, &SM);
----------------
values are usually not annotated with `const`, please remove it for consistency


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:122
+
+  // Now, this is troubling. The literal may be somehow expanded from macro.
+  // So we can't just use plain getSourceRange(). An assumption is made that one
----------------
please remove the `Now this is troubling`.
Please use `two or more (what, macros?)` instead of `two+`


================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:192
+bool UppercaseLiteralSuffixCheck::checkBoundMatch(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Literal =
----------------
`ast_matchers::` is not necessary, because there is a `using ...` at the top of the file


================
Comment at: clang-tidy/utils/ASTUtils.cpp:72
+bool rangeIsEntirelyWithinMacroArgument(SourceRange Range,
+                                        const SourceManager *SM) {
+  // Check if the range is entirely contained within a macro argument.
----------------
Does it make sense to have a `nullptr` for SM? I think you can use a reference here


================
Comment at: docs/clang-tidy/checks/list.rst:67
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
----------------
hicpp alias is missing here


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670