aaron.ballman added a comment.
Two main points: I don't think this check is covering all of the suffixes (I
don't see `q` or `i32` support, for instance), and at least for the CERT rule
this is covering, it diagnoses more than it should. The CERT rule is specific
to `l` vs `L` but imposes no requirements on `u` vs `U`.
================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:88-89
"readability-string-compare");
+ CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
+ "readability-uppercase-literal-suffix");
CheckFactories.registerCheck<readability::NamedParameterCheck>(
----------------
Please keep this sorted alphabetically.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:25
+
+struct IntegerLiteral {
+ using type = clang::IntegerLiteral;
----------------
It's unfortunate that clang::IntegerLiteral and
clang::tidy::readability::IntegerLiteral are distinct types that are strongly
related to one another. I'm not keen on this name, as it means reading the
code, an unqualified IntegerLiteral is hard to reason about.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:30-31
+ static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("");
+ // Integer suffix can only consist of 'u' and 'l' chars.
+ static constexpr llvm::StringLiteral Suffixes = llvm::StringLiteral("uUlL");
+};
----------------
There are other suffixes for integer literals. See `NumericLiteralParser`.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:37
+
+struct FloatingLiteral {
+ using type = clang::FloatingLiteral;
----------------
Same naming concerns here.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:47-48
+ static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("pP");
+ // Floating suffix can only consist of 'u' and 'l' chars.
+ static constexpr llvm::StringLiteral Suffixes = llvm::StringLiteral("fFlL");
+};
----------------
There are more suffixes here (see `NumericLiteralParser` again), and this
comment seems to be copy pasta.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:99-100
+ const SourceManager &SM) {
+ auto Begin = GetMacroAwareLocation(Loc.getBegin(), SM);
+ auto End = GetMacroAwareLocation(Loc.getEnd(), SM);
+ if (!Begin || !End)
----------------
Don't use `auto` here.
================
Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:122
+ // The literal may have macro expansion, we need the final expanded src
range.
+ auto Range = GetMacroAwareSourceRange(ReplacementDsc.LiteralLocation, SM);
+ if (!Range)
----------------
Don't use `auto` here.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52670
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
