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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.

Except readability issues with this check code (bunch of else, ..), and that I didn't took a deep dive into format string conversion (I hope that author is right there).
I would say that this check probably could be delivered. Would be nice to run it on some test projects, just to be sure that it wont crash or wont produce too much false-positives.
I still would expect that probably there can be some issues with it...

Next steps for this check would be:

- Fix some known (breaking) issues (like option naming)
- Deliver check to main
- Refactor code for performance / readability.
- Deliver code refactor.

Once on main branch, check can be tested by other users, and there is a chance that some issues with it could be found before branch-out.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:117
+          *MaybeHeaderToInclude);
+  } else
+    DiagnosticBuilder Diag = diag(PrintfCall->getBeginLoc(),
----------------
Personally I would invert this, `if (!...canApply()) { diag(PrintfCall->getBeginLoc(),  "unable to use '%0' instead of %1 because %2") << ...; return; }`


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:118
+  } else
+    DiagnosticBuilder Diag = diag(PrintfCall->getBeginLoc(),
+                                  "unable to use '%0' instead of %1 because %2")
----------------
no need to assign this to Diag, it will be destroyed anyway, just call diag.


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:27
+
+namespace {
+/// Is the passed type the actual "char" type, whether that be signed or
----------------
no need for anonymous namespace per LLVM codding standard, use static keyword for all functions, put them all into clang::tidy::utils namespace


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:146-148
+namespace clang::ast_matchers {
+AST_MATCHER(QualType, isRealChar) { return isRealCharType(Node); }
+} // namespace clang::ast_matchers
----------------
do not put matchers into clang::ast_matchers to avoid ODR issues. put it into anonymous namespace under clang::tidy::utils


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:157-175
+    : Context(ContextIn), CastMismatchedIntegerTypes([Call, StrictMode]() {
+        /// For printf-style functions, the signedness of the type printed is
+        /// indicated by the corresponding type in the format string.
+        /// std::print will determine the signedness from the type of the
+        /// argument. This means that it is necessary to generate a cast in
+        /// StrictMode to ensure that the exact behaviour is maintained.
+        /// However, for templated functions like absl::PrintF and
----------------
consider moving this into separate function that take Call and StrictMode as an argument this isn't readable. make function static.


================
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")),
----------------
mikecrowe wrote:
> PiotrZSL wrote:
> > constant construction of those marchers may be costly.. can be cache them somehow in this object ?
> Good idea. Do you have any pointers to code that does this? I couldn't find any.
> 
> The types involved all seem to be in the `internal::` namespace and presumably subject to change. Ideally, I'd put:
> ```
> std::optional<SomeSortOfMatcherType> StringCStrCallExprMatcher;
> ```
> in the class and then populate it here the first time lazily. Unfortunately I have no idea what type to use for `SomeSortOfMatcherType`.
`clang::ast_matchers::StatementMatcher<CXXMemberCallExpr>`, this is defined as `using clang::ast_matchers::StatementMatcher = typedef internal::Matcher<Stmt>`.
I would create it in class constructor as private member.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117
+
+.. option:: PrintFunction
+
----------------
mikecrowe wrote:
> Is `PrintFunction` (and the soon-to-arrive `PrintlnFunction`) distinct enough from `PrintfLikeFunctions` and `FprintfLikeFunctions`? Should I use `ReplacementPrintFunction` instead?
Use `Replacement`. It will be way better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280