[PATCH] D154287: [clang-tidy] Add modernize-use-std-format check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

To be honest, I do not see to much use case for this check. But that's fine.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:46-51
+  StatementMatcher StrFormatMatcher =
+      callExpr(callee(functionDecl(matchers::matchesAnyListedName(
+                                       StrFormatLikeFunctions))
+                          .bind("func_decl")),
+               hasArgument(0, stringLiteral()))
+          .bind("StrFormat");
----------------
NOTE: put those things directly into addMatcher


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:50
+                          .bind("func_decl")),
+               hasArgument(0, stringLiteral()))
+          .bind("StrFormat");
----------------
put hasArgument before callee (performance), you can also add `unless(argumentCountIs(0))`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:69
+  const auto *OldFunction = Result.Nodes.getNodeAs<FunctionDecl>("func_decl");
+  const auto *StrFormat = Result.Nodes.getNodeAs<CallExpr>("StrFormat");
+
----------------
keep binding consistent `StrFormat` vs `func_decl`


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:78
+  if (!Converter.canApply()) {
+    DiagnosticBuilder Diag = diag(StrFormatCall->getBeginLoc(),
+                                  "unable to use '%0' instead of %1 because %2")
----------------
Use callExpr location, not calee for diagnostic if possible (should point to same place anyway).


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:81
+                             << ReplacementFormatFunction
+                             << OldFunction->getIdentifier()
+                             << Converter.conversionNotPossibleReason();
----------------
using ``<< OldFunction`` should be sufficient and more safe, same in line 88


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:629-630
+  if (Config.AllowTrailingNewlineRemoval &&
+      StringRef(StandardFormatString).ends_with("\\n") &&
       !StringRef(StandardFormatString).ends_with("\\\\n")) {
     UsePrintNewlineFunction = true;
----------------
what if it's ends with Windows new line style ? Will it work ?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:27
+The check uses the same format-string-conversion algorithm as
+`modernize-use-std-print <modernize/use-std-print.html>` and its
+shortcomings are described in the documentation for that check.
----------------
is this link correct ? shouldn't this be put into `doc:`, verify if its working


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:33
+
+.. option:: StrictMode TODO
+
----------------
If not implemented, do not add it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154287/new/

https://reviews.llvm.org/D154287