JonasToth added a comment. Thanks for working on this!
Related as well: http://www.codingstandard.com/rule/4-2-1-ensure-that-the-u-suffix-is-applied-to-a-literal-used-in-a-context-requiring-an-unsigned-integral-expression/ I think its wort a alias is hicpp as well Please add tests that use user-defined literals and ensure there are no collision and that they are not diagnosed. Some examples: https://en.cppreference.com/w/cpp/language/user_literal ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:26 +struct IntegerLiteral { + using ClangType = clang::IntegerLiteral; + static constexpr llvm::StringLiteral Name = llvm::StringLiteral("integer"); ---------------- maybe `ClangType` is not a good name, how about just `type` to be consistent with e.g. std::vector convention. The use-case in your template makes it clear, that we are talking about `LiteralType`s. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:33 +}; +constexpr llvm::StringLiteral IntegerLiteral::Name; +constexpr llvm::StringLiteral IntegerLiteral::SkipFirst; ---------------- why are these declarations necessary? ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:42 + // What should be skipped before looking for the Suffixes? + // Hexadecimal floating-point literals: skip until exponent first. + static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("pP"); ---------------- The second line of the comment is slightly confusing, please make a full sentence out of it. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:52 +AST_MATCHER(clang::IntegerLiteral, intHasSuffix) { + const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr()); + if (!T) ---------------- as it hit me in my check: what about `(1)ul`? Is this syntactically correct and should be diagnosed too? (Please add tests if so). In this case it should be `Note.getType().IgnoreParens().getTypePtr())`. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:53 + const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr()); + if (!T) + return false; ---------------- 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. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:69 +AST_MATCHER(clang::FloatingLiteral, fpHasSuffix) { + const auto *T = dyn_cast<BuiltinType>(Node.getType().getTypePtr()); + if (!T) ---------------- Same comment as above ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:85 +template <typename LiteralType> +llvm::Optional<UppercaseLiteralSuffixCheck::NewSuffix> +UppercaseLiteralSuffixCheck::shouldReplaceLiteralSuffix( ---------------- These types get really long. Is it possible to put `NewSuffix` into the anonymous namespace as well? ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:90 + + NewSuffix S; + ---------------- GIven that this variable is used mutliple times in a longer function, i feel it shuold get a longer, more descriptive name ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:95 + // Get the whole Integer Literal from the source buffer. + const StringRef LiteralSourceText = Lexer::getSourceText( + CharSourceRange::getTokenRange(S.Range), SM, getLangOpts()); ---------------- Please check if the source text could be retrieved, with a final `bool` parameter, thats in/out and at least `assert` on that. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:106 + Skip = LiteralSourceText.find_first_of(LiteralType::SkipFirst); + if (Skip == StringRef::npos) // We could be in non-hexadecimal fp literal. + Skip = 0; ---------------- please use `flloating-point` instead of `fp` (same in other places) ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:129 + if (S.OldSuffix == S.NewSuffix) + return llvm::None; // The suffix was already fully uppercase. + ---------------- Could this function return a `Optional<FixitHint>`? That would include the range and the relacement-text. I feel that is would simplify the code, especially the amount of state that one has to keep track of. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:135 +void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + stmt(integerLiteral(intHasSuffix())).bind(IntegerLiteral::Name), this); ---------------- I think you can merge this matcher to `stmt(eachOf(integerLiteral(intHasSuffix()).bind(), floatLiteral(fpHasSuffix()).bind()))` `eachOf` because we want to match all, and not short-circuit. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:152 + // Ignore literals that aren't fully written in the source code. + if (LiteralLocation.isMacroID() || + Result.SourceManager->isMacroBodyExpansion(LiteralLocation) || ---------------- Wouldn't it make sense to warn for the literals in macros? ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165 + + diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case") + << LiteralType::Name << S.OldSuffix ---------------- Lets move this `diag` into the true branch of the if stmt and drop the ´else`. ================ Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:165 + + diag(LiteralLocation, "%0 literal suffix '%1' is not upper-case") + << LiteralType::Name << S.OldSuffix ---------------- 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 ================ Comment at: docs/ReleaseNotes.rst:96 +- New alias :doc:`cert-dcl16-c + <clang-tidy/checks/cert-dcl16-c>` to :doc:`readability-uppercase-literal-suffix ---------------- lebedev.ri wrote: > Eugene.Zelenko wrote: > > Please move alias after new checks. > BTW, is there some tool to actually add this alias? I didn't find it, and had > to do it by hand.. So far there is nothing, but would be a nice addition :) ================ Comment at: docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst:9 +Detects when the integral literal or floating point (decimal or hexadecimal) +literal has non-uppercase suffix, and suggests to make the suffix uppercase, +with fix-it. ---------------- I feel that the sentence could be improved a bit. - `literal has *a* non-uppercase` - `suffix and provides a fix-it-hint with the uppercase suffix.` (i think the comma after `suffix` is not necessary) ================ Comment at: test/clang-tidy/readability-uppercase-literal-suffix-floating-point.cpp:26 + static_assert(is_same<decltype(v0), const double>::value, ""); + static_assert(v0 == 1, ""); + ---------------- nit: comparing int to double, same below. ================ Comment at: test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp:6 + +template <class T, T v> +struct integral_constant { ---------------- Please remove the TMP duplication and make a extra header for that, that is then included by these tests. ================ Comment at: test/clang-tidy/readability-uppercase-literal-suffix-hexadecimal-floating-point.cpp:31 + static constexpr auto v1 = 0xfp0f; + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: floating point literal suffix 'f' is not upper-case [readability-uppercase-literal-suffix] + // CHECK-MESSAGES-NEXT: static constexpr auto v1 = 0xfp0f; ---------------- you can safely drop the `[readability-..]` part of the warning message. It will just make problems if the check should be renamed at some point. 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
