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

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe marked 9 inline comments as done.
mikecrowe added a comment.

Thanks for the reviews!



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:81
+                             << ReplacementFormatFunction
+                             << OldFunction->getIdentifier()
+                             << Converter.conversionNotPossibleReason();
----------------
PiotrZSL wrote:
> using ``<< OldFunction`` should be sufficient and more safe, same in line 88
I don't think that's the same. If I use just `OldFunction` then I get the template arguments in strings like `StrFormat<const char *, const char *, const char *, const char *>` and `sprintf<char[6], int>` when I just want `StrFormat` and `sprintf` respectively.


================
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;
----------------
PiotrZSL wrote:
> what if it's ends with Windows new line style ? Will it work ?
Good point. Your question actually relates to the `modernize-use-std-print` check only since `config.AllowTrailingNewlineRemoval` is always `false` in this check.

`std::println` is [[ https://en.cppreference.com/w/cpp/io/println | described ]] as just appending `\n` to the end of the formatted string when written to the desired output. The code here means that `printf("A\r\n")` would be converted to `std::println("A\r")` which would behave in the same way, though I would admit that it's rather confusing.

This situation isn't really Windows-specific. Most the of the time a POSIX tty would be in cooked mode, so the `\r` would be added automatically before the `\n`. However, when the tty is in raw mode using `\r\n` line endings would be necessary there too, just as they might be on Windows when writing to a binary stream.

I think that the least confusing outcome would be for this code to not consider format strings that end with `\r\n`  to be suitable for conversion to `std::println` and let them use `std::print` instead. I think that this better reflects the intent of the existing code.

I will prepare a separate change for this.


================
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.
----------------
PiotrZSL wrote:
> is this link correct ? shouldn't this be put into `doc:`, verify if its working
It was wrong, but I believe that I've fixed it.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst:33
+
+.. option:: StrictMode TODO
+
----------------
PiotrZSL wrote:
> If not implemented, do not add it
It is implemented, I just forgot to remove the TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154287