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