PiotrZSL added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:43-44
+
+ if (!MaybeHeaderToInclude && (ReplacementPrintFunction == "std::print" ||
+ ReplacementPrintlnFunction == "std::println"))
+ MaybeHeaderToInclude = "<print>";
----------------
this code is already duplicated, move it to some separate private function,
like isCpp23PrintConfigured or something...
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:56
+ Finder->addMatcher(
+ traverse(TK_IgnoreUnlessSpelledInSource,
+ callExpr(callee(functionDecl(matchers::matchesAnyListedName(
----------------
Those TK_IgnoreUnlessSpelledInSource should be moved into
getCheckTraversalKind()
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:33
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
----------------
missing store function, to properly export configuration via --export-config
================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:40
+ StringRef ReplacementPrintlnFunction;
+ utils::IncludeInserter IncludeInserter;
+ std::optional<StringRef> MaybeHeaderToInclude;
----------------
I heard that using clang::tooling::HeaderIncludes is more recommended.
And this is just some house-made stuff
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:37-39
+ } else {
+ return false;
+ }
----------------
NOTE: no need fore else or {}
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:185
+ assert(FormatExpr);
+ PrintfFormatString = FormatExpr->getString();
+
----------------
This may crash for wchar, maybe better limit StringLiteral to Ordinary and UTF8
or only Ordinary
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:189
+ // but perhaps with a few escapes expanded.
+ StandardFormatString.reserve(PrintfFormatString.size() + 8);
+ StandardFormatString.push_back('\"');
----------------
magic number
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:202
+/// Called for each format specifier by ParsePrintfString.
+bool FormatStringConverter::HandlePrintfSpecifier(
+ const analyze_printf::PrintfSpecifier &FS, const char *StartSpecifier,
----------------
this function should be split into smaller one.
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:230
+ return conversionNotPossible("'%m' is not supported in format string");
+ } else {
+ StandardFormatString.push_back('{');
----------------
general: no need for else after return
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:384-397
+ const auto StringDecl = type(hasUnqualifiedDesugaredType(recordType(
+ hasDeclaration(cxxRecordDecl(hasName("::std::basic_string"))))));
+ const auto StringExpr = expr(anyOf(
+ hasType(StringDecl), hasType(qualType(pointsTo(StringDecl)))));
+
+ const auto StringCStrCallExpr =
+ cxxMemberCallExpr(on(StringExpr.bind("arg")),
----------------
constant construction of those marchers may be costly.. can be cache them
somehow in this object ?
================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:418
+ case ConversionSpecifier::Kind::iArg:
+ if (Arg->getType()->isBooleanType()) {
+ // std::format will print bool as either "true" or "false" by
default,
----------------
maybe put that getType into some local variable, instead of constantly
referencing it...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149280/new/
https://reviews.llvm.org/D149280
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
