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