[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Piotr Zegar via Phabricator via cfe-commits]

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