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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
