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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
