[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/IdentifierNamingCheck.cpp:660-678
-  // Check if the range is entirely contained within a macro argument.
-  SourceLocation MacroArgExpansionStartForRangeBegin;
-  SourceLocation MacroArgExpansionStartForRangeEnd;
-  bool RangeIsEntirelyWithinMacroArgument =
-      SourceMgr &&
-      SourceMgr->isMacroArgExpansion(Range.getBegin(),
-                                     &MacroArgExpansionStartForRangeBegin) &&
----------------
@JonasToth that is what this code did.
>From a quick look i'm not sure what it would decide without the source manager,
so i simply refactored it as a function, and kept the semantics.


================
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:
> > 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 :)
See `test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp`, `horrible_macros()`
I guess if we point at the suffix, we will also get less useful `expanded from` messages.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670