lebedev.ri added inline comments.
================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53 + const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr()); + if (!T) + return false; ---------------- JonasToth wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > Maybe the if could init `T`? It would require a second `return false;` if > > > i am not mistaken, but looks more regular to me. No strong opinion from > > > my side. > > Then we will not have an early-return, which is worse than this. > Ok. Can the `dyn_cast` be null at all? Shouldn't integerliteral always be a > `BuiltinType`? I don't know? I would guess no. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85 +template <typename LiteralType> +llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix> +UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix( ---------------- JonasToth wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > These types get really long. Is it possible to put `NewSuffix` into the > > > anonymous namespace as well? > > No, because `shouldReplaceLiteralSuffix()` is a member function which > > returns that type. > > I think it should stay a member function, so theoretically `NewSuffix` > > could be a [second] template param, but that is kinda ugly.. > > I also can't simplify it via `using` because the `NewSuffix` is private. > > > > Perhaps we should keep this as-is? > Why does it need to be a member? It looks like it only accesses `NewSuffix` > which does nothing that requires private access to the check class. Passing `LangOpts` turned out to be sufficient to move it into anon namespace. ================ 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: > 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? Repository: rCRT Compiler Runtime https://reviews.llvm.org/D52670 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
