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

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

2023-06-26 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL added a comment.

In D149280#4450418 , @mikecrowe wrote:

> Despite the fact that I've worked on what became this check for about two 
> years, now that it's landed I've suddenly spotted a significant flaw: 
> `printf` returns the number of characters printed, whereas `std::print` 
> doesn't return anything. None of my test cases made use of the return value. 
> I think this means that I need to only match on calls that don't make use of 
> the return value. I shall try to do that.

You can provide warning, but without fix. But that would need to be put into 
documentation.
If we lucky, and there will be no issues with current version, then you can put 
this in separate patch.
Anyway I do not think that many project use `printf`, most probably they use 
`std::cout`, `fmt::format` or some other custom wrapper around something.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-26 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added a comment.

Despite the fact that I've worked on what became this check for about two 
years, now that it's landed I've suddenly spotted a significant flaw: `printf` 
returns the number of characters printed, whereas `std::print` doesn't return 
anything. None of my test cases made use of the return value. I think this 
means that I need to only match on calls that don't make use of the return 
value. I shall try to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-26 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:169
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 
'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println("Integer {:d} from char", c);
+

This should have a cast to `signed char` if `StrictMode=true`. That cast is 
currently there by accident on targets where `char` is unsigned, but not on 
targets where `char` is signed. This is a real problem, but luckily one that's 
easy to fix.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-26 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked an inline comment as done.
mikecrowe added a comment.

In D149280#4448158 , @PiotrZSL wrote:

> Test is failing:
> https://lab.llvm.org/buildbot/#/builders/230/builds/14939/steps/6/logs/FAIL__Clang_Tools__use-std-print_cpp
> https://lab.llvm.org/buildbot/#/builders/245/builds/10266

It  looks like the `char` test is failing on targets where unadorned `char` is 
`unsigned`. I have access to two such machines, so I ought to be able to 
reproduce this myself (either on the up-to-date slow one, or the fast one that 
lacks a sufficiently-new cmake.)

@thakis, http://45.33.8.238/win/80313/summary.html makes it looks like Windows 
may be happy now, but I may be misinterpreting the results.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-26 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL added a comment.

Test is failing:
https://lab.llvm.org/buildbot/#/builders/230/builds/14939/steps/6/logs/FAIL__Clang_Tools__use-std-print_cpp
https://lab.llvm.org/buildbot/#/builders/245/builds/10266


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-26 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstddef:12
 
-typedef __typeof__(sizeof(0)) size_t;
+#include "stddef.h"
 

We shouldn't include this stddef.h, it's being included from system headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

None i know of, but given the bot's already red, you won't make it worse :)

Tests shouldn't include any headers generally, not even inttypes.h (unless the 
test tests that header).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked an inline comment as done.
mikecrowe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:9-15
+#include 
+#include 
+#include 
+// CHECK-FIXES: #include 
+#include 
+#include 
+#include 

PiotrZSL wrote:
> Those includes need to be removed. We cannot use system includes, some dummy 
> one can be used only.
I believe that `` is the only one that isn't present in 
`test/clang-tidy/checkers/Inputs/Headers/`. Hopefully adding it will have been 
sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added a comment.

In D149280#4447363 , @thakis wrote:

> This breaks tests on windows: http://45.33.8.238/win/80283/step_8.txt
>
> Please take a look and revert for now if it takes a while to fix.

Thanks for letting me know, I thought I could get away with including 
 from a test since it is a compiler header rather than a libc 
header.

I've added a potential fix, but I don't have a Windows machine (or many other 
targets) to test it on. Is there a way to get this change to run through the 
buildbots without landing it?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Piotr Zegar via Phabricator via cfe-commits

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

@thakis Thank you.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:9-15
+#include 
+#include 
+#include 
+// CHECK-FIXES: #include 
+#include 
+#include 
+#include 

Those includes need to be removed. We cannot use system includes, some dummy 
one can be used only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

This breaks tests on windows: http://45.33.8.238/win/80283/step_8.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:64
+  if (MaybeHeaderToInclude)
+Options.store(Opts, "PrintHeader", *MaybeHeaderToInclude);
+}

This is going to write the default value set in the constructor if 
`ReplacementPrintFunction` is `std::print` or `ReplacementPrintlnFunction` is 
`std::println` even if `PrintHeader` was not specified in the configuration. I 
don't know how much of a problem this is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-25 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL accepted this revision.
PiotrZSL added a comment.

Consider delivering this check. I do not think that it will become much better 
with more refactoring.




Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:202-203
+  assert(FormatExpr);
+  if (!FormatExpr->isOrdinary())
+return; // No wide string support yet
+  PrintfFormatString = FormatExpr->getString();

this check for isOrdinary could be done on matcher level, just add anonymous 
matcher, and use it there, you can still use it also here, but my idea is to 
reduce amount of calls to check method.
```
AST_MATCHER(StringLiteral, isOrdinary) {
  return Node.isOrdinary();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Mike Crowe via Phabricator via cfe-commits

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

Thanks for the further reviews.




Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:37
+
+  if (PrintfLikeFunctions.empty() && FprintfLikeFunctions.empty()) {
+PrintfLikeFunctions.push_back("::printf");

PiotrZSL wrote:
> those 2 looks in-depended. Cannot they be put as default value into Option 
> config ?
My expectation was that if you didn't want to replace `printf` (and instead 
wanted to replace some other `printf`-like function) then you'd also not want 
to replace `fprintf` and vice-versa. In my head the two options do go together. 
If setting one didn't also remove the default for the other you'd end up having 
to specify `PrintfLikeFunctions=my_printf, FPrintfLikeFunctions=` just to 
replace a single function.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:79
+  Finder->addMatcher(
+  callExpr(callee(functionDecl(
+  matchers::matchesAnyListedName(FprintfLikeFunctions))

PiotrZSL wrote:
> this could match also methods, maybe something `unless(cxxMethodDecl())` ?
I would like to be able to match methods. The code I wrote this check to run on 
has classes that have methods that used to call `std::fprintf`, and currently 
call `fmt::fprintf`. I wish to convert those methods to use `fmt::print` and 
use this check to fix all their callers. Eventually I hope to be able to move 
to `std::print`.

However, the check doesn't currently seem to work for methods since the 
arguments are off-by-one and the replacement is incorrect too. I'll add the 
`unless(cxxMethodDecl)` checks for now and support can be added later.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:82
+  .bind("func_decl")),
+   hasArgument(1, stringLiteral()))
+  .bind("fprintf"),

PiotrZSL wrote:
> consider moving this stringLiteral to be first, consider adding something 
> like `unless(argumentCountIs(0)), unless(argumentCountIs(1))`` at begining or 
> create matcher that verify first that argument count is >= 2, so we could 
> exlude most of callExpr at begining, even before we match functions
I would have hoped that `hasArgument` would fail quite fast if the argument 
index passed is greater than the number of arguments. I will add 
`argumentCountAtLeast` and use it regardless though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL accepted this revision.
PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:37
+
+  if (PrintfLikeFunctions.empty() && FprintfLikeFunctions.empty()) {
+PrintfLikeFunctions.push_back("::printf");

those 2 looks in-depended. Cannot they be put as default value into Option 
config ?



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:70
+void UseStdPrintCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  callExpr(callee(functionDecl(

call this function only if PrintfLikeFunctions is not empty.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:78
+
+  Finder->addMatcher(
+  callExpr(callee(functionDecl(

call this function only if FprintfLikeFunctions is not empty



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:79
+  Finder->addMatcher(
+  callExpr(callee(functionDecl(
+  matchers::matchesAnyListedName(FprintfLikeFunctions))

this could match also methods, maybe something `unless(cxxMethodDecl())` ?



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:82
+  .bind("func_decl")),
+   hasArgument(1, stringLiteral()))
+  .bind("fprintf"),

consider moving this stringLiteral to be first, consider adding something like 
`unless(argumentCountIs(0)), unless(argumentCountIs(1))`` at begining or create 
matcher that verify first that argument count is >= 2, so we could exlude most 
of callExpr at begining, even before we match functions



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:58
+- ``printf``, ``fprintf``, ``absl::PrintF``, ``absl::FPrintF`` and any
+  functions specified by the ``PrintfLikeFunctions`` option or
+  ``FprintfLikeFunctions`` are replaced with the function specified by the





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:59
+  functions specified by the ``PrintfLikeFunctions`` option or
+  ``FprintfLikeFunctions`` are replaced with the function specified by the
+  ``ReplacementPrintlnFunction`` option if the format string ends with





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:60
+  ``FprintfLikeFunctions`` are replaced with the function specified by the
+  ``ReplacementPrintlnFunction`` option if the format string ends with
+  ``\n`` or ``ReplacementPrintFunction`` otherwise.





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:61
+  ``ReplacementPrintlnFunction`` option if the format string ends with
+  ``\n`` or ``ReplacementPrintFunction`` otherwise.
+- the format string is rewritten to use the ``std::formatter`` language and




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added a comment.

In D149280#4446591 , @mikecrowe wrote:

> @Eugene.Zelenko,
> I will make the documentation changes you've suggested, but I can't say I 
> really understand which document elements require single backticks and which 
> require dual backticks. https://llvm.org/docs/SphinxQuickstartTemplate.html 
> seems to imply that single backticks are only used for links and double 
> backticks are used for monospace, but it's not completely clear. Most of the 
> existing documentation seems to be consistent with what you've suggested 
> though.

In short: double back-ticks are for language constructs. Single - for 
command-line options, options and their values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added a comment.

@Eugene.Zelenko,
I will make the documentation changes you've suggested, but I can't say I 
really understand which document elements require single backticks and which 
require dual backticks. https://llvm.org/docs/SphinxQuickstartTemplate.html 
seems to imply that single backticks are only used for links and double 
backticks are used for monospace, but it's not completely clear. Most of the 
existing documentation seems to be consistent with what you've suggested though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:29
+- It assumes that the format string is correct for the arguments. If you
+  get any warnings when compiling with ``-Wformat`` then misbehaviour is
+  possible.

Please use single back-ticks for `-Wformat` (command-line option).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:81
+
+   When true, the check will add casts when converting from variadic
+   functions like ``printf`` and printing signed or unsigned integer types

Please highlight `true` with back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:87
+   converting from non-variadic functions such as ``absl::PrintF`` and
+   ``fmt::printf``. For example, with ``StrictMode`` enabled:
+

Please use single back-ticks for `StrictMode` (option).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:103
+  of -42 and the signed representation of 0x (often 4294967254
+  and -1 respectively.) When false (which is the default), these casts
+  will not be added which may cause a change in the output.

Please highlight `false` and numbers with back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:112
+   immediately afterwards. If neither this option nor
+   ``FprintfLikeFunctions`` are set then the default value for this option
+   is ``printf; absl::PrintF``, otherwise it is empty.

Please use single back-ticks for `FprintfLikeFunctions` (option). Same for 
`printf; absl::PrintF` below.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:122
+   arguments to be formatted follow immediately afterwards. If neither this
+   option nor ``PrintfLikeFunctions`` are set then the default value for
+   this option is ``fprintf; absl::FPrintF``, otherwise it is empty.

Ditto.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:144
+   The header that must be included for the declaration of
+   ``ReplacementPrintFunction`` so that a ``#include`` directive can be
+   added if required. If ``ReplacementPrintFunction`` is ``std::print``

Please use single back-ticks for `ReplacementPrintFunction` (option).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked 4 inline comments as done.
mikecrowe added inline comments.



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")),

PiotrZSL wrote:
> 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 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`, this is defined 
> as `using clang::ast_matchers::StatementMatcher = typedef 
> internal::Matcher`.
> I would create it in class constructor as private member.
Thanks for the pointer. It turns out that I need just 
`clang::ast_matchers::StatementMatcher`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117
+
+.. option:: PrintFunction
+

mikecrowe wrote:
> PiotrZSL wrote:
> > 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.
> I assume that you mean `ReplacementPrintFunction`. (I see `ReplacementString` 
> used in other checks, so I think that just `Replacement` might be a bit 
> vague.)
Yes, ... `ReplacementPrintFunction`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-24 Thread Mike Crowe via Phabricator via cfe-commits

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

I'm still working on the remaining items.




Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:230
+return conversionNotPossible("'%m' is not supported in format string");
+  } else {
+StandardFormatString.push_back('{');

mikecrowe wrote:
> PiotrZSL wrote:
> > general: no need for else after return
> In general, you're correct that I've used else after return in other places 
> and I will fix them. However, in this case the else is required since the 
> very first `if` block above doesn't have a return. (Hopefully this complexity 
> will go away if I succeed in splitting this function up.)
I've now reworked this function extensively, so there's no longer an else here.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117
+
+.. option:: PrintFunction
+

PiotrZSL wrote:
> 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.
I assume that you mean `ReplacementPrintFunction`. (I see `ReplacementString` 
used in other checks, so I think that just `Replacement` might be a bit vague.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-18 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



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 , const char *StartSpecifier,

mikecrowe wrote:
> PiotrZSL wrote:
> > this function should be split into smaller one.
> I agree. I'm surprised it hasn't come up in review earlier. I tried to do so 
> prior to pushing the very first version of this for review and gave up since 
> the parts ended up referring to so many local variables making the code even 
> harder to understand than it is now. I will have another attempt.
It turns out that things are working out much better in my second attempt at 
splitting this function. I shall address the other new review comments before 
sending another revision for review though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-06-18 Thread 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 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`, this is defined as 
`using clang::ast_matchers::StatementMatcher = typedef internal::Matcher`.
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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

2023-06-18 Thread Mike Crowe via Phabricator via cfe-commits

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

Thanks for the comprehensive review.




Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:43-44
+
+  if (!MaybeHeaderToInclude && (ReplacementPrintFunction == "std::print" ||
+ReplacementPrintlnFunction == "std::println"))
+MaybeHeaderToInclude = "";

PiotrZSL wrote:
> this code is already duplicated, move it to some separate private function, 
> like isCpp23PrintConfigured or something...
You're correct that `ReplacementPrintFunction == "std::print" || 
ReplacementPrintlnFunction == "std::println"` is duplicated in 
`isLanguageVersionSupported`, but in that code we're considering which C++ 
version the functions require and in this code we're considering which header 
the functions require. These are not the same question. It just so happens that 
these checks are against the same function names now, but they need not be. If 
another function is added to `` in C++26 and this code ends up being 
able to convert to it then the checks would need to diverge.

Nevertheless, I will do as you request if you still think it's worthwhile.




Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:56
+  Finder->addMatcher(
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   callExpr(callee(functionDecl(matchers::matchesAnyListedName(

PiotrZSL wrote:
> Those TK_IgnoreUnlessSpelledInSource should be moved into 
> getCheckTraversalKind()
Ah, that was added since I wrote the original version of this check. I'll 
address this.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:40
+  StringRef ReplacementPrintlnFunction;
+  utils::IncludeInserter IncludeInserter;
+  std::optional MaybeHeaderToInclude;

PiotrZSL wrote:
> I heard that using clang::tooling::HeaderIncludes is more recommended.
> And this is just some house-made stuff
I'm afraid that I don't really understand this comment. The use of 
`IncludeInserter` is far more common in the existing checks than 
`HeaderIncludes` and from looking at `IncludeCleaner.cpp` the use of the latter 
is rather more complex. New features were being added to `IncludeInserter` 
within the last six months. I'm unsure what "house-made" means in the context 
of the `IncludeInserter` code that I didn't write.

Do you have anything more concrete than hearsay? If there is a plan to 
deprecate `IncludeInserter` then it feels like something closer to its 
convenient interface would need to be implemented in terms of `HeaderIncludes` 
for checks to use rather than them duplicating the code in `IncludeCleaner.cpp`.

If you can explain what you'd like me to do, and ideally provide pointers to 
similar things being done elsewhere then perhaps I can address this comment.



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:185
+  assert(FormatExpr);
+  PrintfFormatString = FormatExpr->getString();
+

PiotrZSL wrote:
> This may crash for wchar, maybe better limit StringLiteral to Ordinary and 
> UTF8 or only Ordinary 
It doesn't look like there's any support for u8 literals in `printf` or 
`std::print` (yet), so I think just Ordinary is sufficient.



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 , const char *StartSpecifier,

PiotrZSL wrote:
> this function should be split into smaller one.
I agree. I'm surprised it hasn't come up in review earlier. I tried to do so 
prior to pushing the very first version of this for review and gave up since 
the parts ended up referring to so many local variables making the code even 
harder to understand than it is now. I will have another attempt.



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:230
+return conversionNotPossible("'%m' is not supported in format string");
+  } else {
+StandardFormatString.push_back('{');

PiotrZSL wrote:
> general: no need for else after return
In general, you're correct that I've used else after return in other places and 
I will fix them. However, in this case the else is required since the very 
first `if` block above doesn't have a return. (Hopefully this complexity will 
go away if I succeed in splitting this function up.)



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

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

2023-06-13 Thread 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 = "";

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 ) 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 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 , 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-31 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:79
+
+.. option:: StrictMode
+

mikecrowe wrote:
> It turns out that absl::PrintF and absl::FPrintF work like std::format, 
> fmt::printf, etc. and use the signedness of the argument rather than the 
> signedness indicated in the format string to determine how to format the 
> number. In other words:
> `unsigned int ui = 0x; absl::PrintF("%d\n", ui);` yields `4294967295` 
> (see https://godbolt.org/z/dYcbehxP9 ), so the casts that StrictMode adds 
> would change the behaviour of the converted code.
> 
> I can think of several ways around this:
> 
> 1. Update the documentation to explain this, recommending not to use 
> StrictMode when converting Abseil functions.
> 
> 2. Remove built-in support for Abseil functions from this check. Anyone 
> wishing to convert them can do so via the customisation features, and can 
> choose not to use StrictMode. (This could be documented here.)
> 
> 3. Teach the code to recognise whether the arguments is being passed as a 
> C-style variable argument list or as fully-typed arguments to a templated 
> function and make StrictMode only add the casts for the former. (I've not 
> investigated how feasible this is.)
> 
> 4. Treat the known Abseil functions in this check differently by name and 
> disable the casting behaviour. This means that conversion from fmt::printf 
> via the customisation mechanism wouldn't automatically get that behaviour.
> 
> As someone who doesn't use Abseil I'm inclined towards option 2, with the 
> possibility of implementing option 3 in a separate commit later. I'm not 
> particularly keen on the other two options.
It turns out that option 3 was easier to implement than I expected, so I've 
implemented it in the latest version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-20 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:79
+
+.. option:: StrictMode
+

It turns out that absl::PrintF and absl::FPrintF work like std::format, 
fmt::printf, etc. and use the signedness of the argument rather than the 
signedness indicated in the format string to determine how to format the 
number. In other words:
`unsigned int ui = 0x; absl::PrintF("%d\n", ui);` yields `4294967295` 
(see https://godbolt.org/z/dYcbehxP9 ), so the casts that StrictMode adds would 
change the behaviour of the converted code.

I can think of several ways around this:

1. Update the documentation to explain this, recommending not to use StrictMode 
when converting Abseil functions.

2. Remove built-in support for Abseil functions from this check. Anyone wishing 
to convert them can do so via the customisation features, and can choose not to 
use StrictMode. (This could be documented here.)

3. Teach the code to recognise whether the arguments is being passed as a 
C-style variable argument list or as fully-typed arguments to a templated 
function and make StrictMode only add the casts for the former. (I've not 
investigated how feasible this is.)

4. Treat the known Abseil functions in this check differently by name and 
disable the casting behaviour. This means that conversion from fmt::printf via 
the customisation mechanism wouldn't automatically get that behaviour.

As someone who doesn't use Abseil I'm inclined towards option 2, with the 
possibility of implementing option 3 in a separate commit later. I'm not 
particularly keen on the other two options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-14 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:27
+  using namespace clang;
+  if (const auto *BT = llvm::dyn_cast(Ty)) {
+const bool result = (BT->getKind() == BuiltinType::Char_U ||

This apparently need to be `Ty->getUnqualifiedDesugaredType()` to ensure that 
arguments like std::string().c_str() are correctly treated as being of `char` 
type. Without, the `dyn_cast` fails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-11 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117
+
+.. option:: PrintFunction
+

Is `PrintFunction` (and the soon-to-arrive `PrintlnFunction`) distinct enough 
from `PrintfLikeFunctions` and `FprintfLikeFunctions`? Should I use 
`ReplacementPrintFunction` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-08 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe updated this revision to Diff 520439.
mikecrowe added a comment.

Address many more review comments, including:

- Only add casts for signed/unsigned discrepancy if StrictMode option is set.
- Use IncludeInserter to add  or other required header.

Review comments still outstanding:

- Use println if format string ends in `\n`.
- Remove c_str() as part of the same check (I'm not really sure how to do this, 
are there any other checks that I should look at for inspiration?)
- Emit warnings to explain why if conversion is not possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstdio
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdio.h
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-fmt.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
@@ -0,0 +1,1049 @@
+// RUN: %check_clang_tidy -check-suffixes=,STRICT \
+// RUN:   -std=c++2b %s modernize-use-std-print %t -- \
+// RUN:   -config="{CheckOptions: [{key: StrictMode, value: true}]}" \
+// RUN:   -- -isystem %clang_tidy_headers
+// RUN: %check_clang_tidy -check-suffixes=,NOTSTRICT \
+// RUN:   -std=c++2b %s modernize-use-std-print %t -- \
+// RUN:   -config="{CheckOptions: [{key: StrictMode, value: false}]}" \
+// RUN:   -- -isystem %clang_tidy_headers
+#include 
+// CHECK-FIXES: #include 
+#include 
+#include 
+
+void printf_simple() {
+  printf("Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Hello");
+}
+
+void fprintf_simple() {
+  fprintf(stderr, "Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print(stderr, "Hello");
+}
+
+void std_printf_simple() {
+  std::printf("std::Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("std::Hello");
+}
+
+void printf_escape() {
+  printf("before \t");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before \t");
+
+  printf("\n after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("\n after");
+
+  printf("before \a after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before \a after");
+
+  printf("Bell\a%dBackspace\bFF%s\fNewline\nCR\rTab\tVT\vEscape\x1b\x07%d", 42, "string", 99);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Bell\a{}Backspace\bFF{}\fNewline\nCR\rTab\tVT\vEscape\x1b\a{}", 42, "string", 99);
+
+  printf("not special \x1b\x01\x7f");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("not special \x1b\x01\x7f");
+}
+
+void printf_percent() {
+  printf("before %%");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before %");
+
+  printf("%% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("% after");
+
+  printf("before %% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before % after");
+
+  printf("Hello %% and another %%");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Hello % and another

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

2023-05-08 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked 3 inline comments as done.
mikecrowe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:124-126
+  printf("Integer %d from bool\n", b);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 
'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Integer {:d} from bool\n", b);

njames93 wrote:
> It would be nice if the there was support for changing this to 
> `std::println("Integer {:d} from bool", b);`
Yes. That was item 2 on the plans for the future in my first comment. Would you 
prefer that everything is completed in this single commit, or would you be 
happy to the core functionality first with future enhancements in separate 
commits afterwards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-05-06 Thread Nathan James via Phabricator via cfe-commits

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:20
+if (ReplacementPrintFunction == "std::print")
+  return LangOpts.CPlusPlus2b;
+return LangOpts.CPlusPlus;

This will need rebasing and changing to `LangOpts.CPlusPlus23`



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:408-413
+if (Arg->getType()->isNullPtrType())
+  ; // std::format knows how to format nullptr
+else if (Arg->getType()->isVoidPointerType())
+  ; // std::format knows how to format void pointers
+else
+  ArgFixes.emplace_back(Arg, "static_cast(");

Is it not nicer to just search for the compliment



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.h:57
+
+  virtual bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier ,
+ const char *startSpecifier,

Don't need to specify virtual here when the function is marked override



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:44-45
+
+- It is assumed that the  (or other appropriate) header has
+  already been included. No attempt is made to include it.
+

This should be relatively easy to address, just look at how other checks use 
the `IncludeInserter` to add includes.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:124-126
+  printf("Integer %d from bool\n", b);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 
'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Integer {:d} from bool\n", b);

It would be nice if the there was support for changing this to 
`std::println("Integer {:d} from bool", b);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-30 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe updated this revision to Diff 518315.
mikecrowe added a comment.

I've addressed most of the review comments. The work that remains outstanding 
is:

- Emit warnings when conversion is not possible explaining why.

- Only add signed/unsigned casts when in StrictMode.

- Automatically remove now-unnecessary calls to c_str() in arguments rather 
than requiring readability-redundant-string-cstr check to be run afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstdio
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdio.h
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-absl.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-custom.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print-fmt.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
@@ -0,0 +1,1021 @@
+// RUN: %check_clang_tidy -std=c++2b %s modernize-use-std-print %t -- -- -isystem %clang_tidy_headers
+
+#include 
+#include 
+#include 
+
+void printf_simple() {
+  printf("Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Hello");
+}
+
+void fprintf_simple() {
+  fprintf(stderr, "Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print(stderr, "Hello");
+}
+
+void std_printf_simple() {
+  std::printf("std::Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("std::Hello");
+}
+
+void printf_escape() {
+  printf("before \t");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before \t");
+
+  printf("\n after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("\n after");
+
+  printf("before \a after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before \a after");
+
+  printf("Bell\a%dBackspace\bFF%s\fNewline\nCR\rTab\tVT\vEscape\x1b\x07%d", 42, "string", 99);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Bell\a{}Backspace\bFF{}\fNewline\nCR\rTab\tVT\vEscape\x1b\a{}", 42, "string", 99);
+
+  printf("not special \x1b\x01\x7f");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("not special \x1b\x01\x7f");
+}
+
+void printf_percent() {
+  printf("before %%");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before %");
+
+  printf("%% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("% after");
+
+  printf("before %% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("before % after");
+
+  printf("Hello %% and another %%");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Hello % and another %");
+
+  printf("Not a string %%s");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Not a string %s");
+}
+
+void printf_curlies() {
+  printf("%d {}", 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("{} {{[{][{]", 42);
+
+  printf("{}");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use

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

2023-04-30 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp:289
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 
'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("Integer {} from signed char\n", sc);
+

mikecrowe wrote:
> This requires a cast to `unsigned char`. It appears that 
> `Arg->getType()->isSignedIntegerType()` inside the `uArg` case is returning 
> `false` for `signed char`, which means that it doesn't get one. Strange!
> This requires a cast to `unsigned char`. It appears that 
> `Arg->getType()->isSignedIntegerType()` inside the `uArg` case is returning 
> `false` for `signed char`, which means that it doesn't get one. Strange!

Operator error. Apologies for the noise. :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-30 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked an inline comment as done.
mikecrowe added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp:289
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 
'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("Integer {} from signed char\n", sc);
+

This requires a cast to `unsigned char`. It appears that 
`Arg->getType()->isSignedIntegerType()` inside the `uArg` case is returning 
`false` for `signed char`, which means that it doesn't get one. Strange!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-30 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe marked 4 inline comments as done.
mikecrowe added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:61
+- any arguments where the format string and the parameter differ in
+  signedness will be wrapped in an approprate ``static_cast``. For example,
+  given ``int i = 42``, then ``printf("%u\n", i)`` will become

njames93 wrote:
> Perhaps these conversions should only be done in StrictMode
I think that's probably a good idea. The casts are ugly, and probably 
unnecessary in many cases. I've implemented it, but I'm currently wrestling 
with the lit tests for it.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

Eugene.Zelenko wrote:
> mikecrowe wrote:
> > Eugene.Zelenko wrote:
> > > Please add information about default value.
> > The current implementation always "fixes" `printf`, `fprintf`, 
> > `absl::PrintF` and `absl::FPrintf`, even when this option is used (see 
> > `UseStdPrintCheck` constructor.) Should the option inhibit this default?
> It may be reasonable to provide possibility to override default.
I think it's likely that anyone using a custom value for either 
`PrintfLikeFunctions` or `FPrintfLikeFunctions` won't want the default function 
list for either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-29 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

mikecrowe wrote:
> Eugene.Zelenko wrote:
> > Please add information about default value.
> The current implementation always "fixes" `printf`, `fprintf`, `absl::PrintF` 
> and `absl::FPrintf`, even when this option is used (see `UseStdPrintCheck` 
> constructor.) Should the option inhibit this default?
It may be reasonable to provide possibility to override default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-29 Thread Mike Crowe via Phabricator via cfe-commits

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

Thank you for all the review comments.




Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:322
+  if (!isRealCharType(Pointee))
+ArgFixes.emplace_back(Arg, "reinterpret_cast(");
+}

njames93 wrote:
> By default this check should not do this conversion, we shouldn't be creating 
> UB in fixes emitted by clang-tidy unless the user explicitly opts in.
> Maybe a good framework moving forward is to still a warning about converting 
> up std::print, but emit a note here saying why the automatic conversation 
> isn't possible.
Good spot. This was wider than I intended. The cast is only supposed to be used 
if the argument is a pointer to `unsigned char` or `signed char`.

I think that I probably ought to make sure that all the places that decide that 
conversion is not possible emit a warning explaining why.



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:412
+else
+  ArgFixes.emplace_back(Arg, "reinterpret_cast(");
+break;

njames93 wrote:
> `reinterpret_cast` is not needed here, a static cast to`const void*` will 
> work.
Good spot. I'm sure I used the right cast in an earlier version... :(



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

Eugene.Zelenko wrote:
> Please add information about default value.
The current implementation always "fixes" `printf`, `fprintf`, `absl::PrintF` 
and `absl::FPrintf`, even when this option is used (see `UseStdPrintCheck` 
constructor.) Should the option inhibit this default?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:92
+
+It is helpful to use the `readability-redundant-string-cstr
+<../readability/redundant-string-cstr.html>` check after this check to

njames93 wrote:
> It may be wise in a future version to just do that conversion anyway. 2 stage 
> fixes are annoying for users to have to use 
I shall investigate how to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-27 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added a comment.

Thanks for all the reviews.

In D149280#4301188 , @njames93 wrote:

> It may be a good idea to create unittests for the formatconverter in 
> `clang-tools-extra/unittests/clang-tidy/`

I started there but had decided that the easiest and most reviewable form for 
the tests was in lit tests since the code that uses `FormatStringConverter` is 
so simple. (Particularly since I was hoping to persuade the inventor of 
`std::format` to review the tests too.) Are you proposing that I should 
duplicate (most of) the lit tests in the unit tests or migrate most of the lit 
tests to unit tests? Are there any existing tests that do similar things that I 
should model them on?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-27 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:66
+  if (Converter.canApply()) {
+const auto *PrintfCall = Printf->getCallee();
+DiagnosticBuilder Diag =

Please don't use `auto` unless type is explicitly stated in same statement or 
iterator.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.h:15
+namespace clang::tidy::modernize {
+/// Documentation goes here.
+///

Should be elaborated or removed.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:171
 
+- New :doc: `modernize-printf-to-std-print
+  ` check.

Please keep alphabetical order (by check name) in this section.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:6
+
+This check is capable of converting calls to printf to calls to std::print
+whilst also modifying the format string appropriately.

Please make first statement same as in Release Notes. Please highlight language 
constructs (like `printf`) with double back-ticks.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:11
+
+ fprintf(stderr, "The %s is %3d\n", answer, value);
+

Please prepend with `.. code-block:: c++` line. Same for other snippets.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+

Please add information about default value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-27 Thread Piotr Zegar via Phabricator via cfe-commits

PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:28
+  ReplacementPrintFunction(Options.get("PrintFunction", "std::print")) {
+  PrintfLikeFunctions.push_back("printf");
+  PrintfLikeFunctions.push_back("absl::PrintF");

NOTE: Consider using fully qualified names, like `::printf`



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:45
+  Finder->addMatcher(
+  traverse(TK_AsIs,
+   callExpr(callee(functionDecl(matchers::matchesAnyListedName(

TK_IgnoreUnlessSpelledInSource should be fine for this.



Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:68
+DiagnosticBuilder Diag =
+diag(PrintfCall->getBeginLoc(), "Replace %0 with '%1'")
+<< OldFunction->getIdentifier() << ReplacementPrintFunction;

start warning with lower case, and maybe something like "use %0 instead of %1" 
(or whatever)



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.h:32-47
+  const ASTContext *Context;
+  bool ConversionPossible = true;
+  bool FormatStringNeededRewriting = false;
+  size_t PrintfFormatStringPos = 0U;
+  StringRef PrintfFormatString;
+
+  const Expr *const *Args;

NOTE: Consider re-ordering private-public, like in other files, to first put 
public stuff, then private.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-27 Thread Nathan James via Phabricator via cfe-commits

njames93 added a comment.

It may be a good idea to create unittests for the formatconverter in 
`clang-tools-extra/unittests/clang-tidy/`

Can this check be renamed to just `modernize-use-std-print`. This sticks with 
the convention of other modernize names and potentially in the future allows us 
to convert ostream operations to use `std::print`




Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:322
+  if (!isRealCharType(Pointee))
+ArgFixes.emplace_back(Arg, "reinterpret_cast(");
+}

By default this check should not do this conversion, we shouldn't be creating 
UB in fixes emitted by clang-tidy unless the user explicitly opts in.
Maybe a good framework moving forward is to still a warning about converting up 
std::print, but emit a note here saying why the automatic conversation isn't 
possible.



Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:412
+else
+  ArgFixes.emplace_back(Arg, "reinterpret_cast(");
+break;

`reinterpret_cast` is not needed here, a static cast to`const void*` will work.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:61
+- any arguments where the format string and the parameter differ in
+  signedness will be wrapped in an approprate ``static_cast``. For example,
+  given ``int i = 42``, then ``printf("%u\n", i)`` will become

Perhaps these conversions should only be done in StrictMode



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:92
+
+It is helpful to use the `readability-redundant-string-cstr
+<../readability/redundant-string-cstr.html>` check after this check to

It may be wise in a future version to just do that conversion anyway. 2 stage 
fixes are annoying for users to have to use 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe added a comment.

If this sort of check is deemed acceptable then I have plans for future 
improvements, including:

1. Automatically turning absl::StrFormat into fmt::format too (with an option 
to choose an alternative to `absl::StrFormat`.)

2. Detecting format strings that end in `"\n"` and turning the call into 
`std::println` rather than `std::print`.

3. It ought to be possible to do something with %m by using `std::error_code`.

4. Support for member functions and even `operator()` for help with converting 
logging class methods.

My attempts to do something sensible with wide characters failed miserably. 
I've never used wprintf etc. myself and it would probably require the skills of 
someone more familiar with them.

Since many other clang-tidy checks are for Abseil, support for `absl::PrintF` 
and `absl::FPrintF` is built in. They could be removed since it would always be 
possible to replace them using the provided options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-04-26 Thread Mike Crowe via Phabricator via cfe-commits

mikecrowe created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
mikecrowe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Add FormatStringConverter utility class that is capable of converting
printf-style format strings into std::format-style format strings along
with recording a set of casts to wrap the arguments as required.

Use FormatStringConverter to implement a new clang-tidy check that is
capable of converting calls to printf, fprintf, absl::PrintF,
absl::FPrintF, or any function configured by an option to calls to
std::print or another function configured by an option.

In other words, the check turns:

fprintf(stderr, "The %s is %3d\n", answer, value);

into:

std::print(stderr, "The {} is {:3}\n", answer, value);

if it can.

std::print can do almost anything that standard printf can, but the
conversion has some some limitations that are described in the
documentation. If conversion is not possible then the call remains
unchanged.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149280

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp
  clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
  clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstdio
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdio.h
  
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-fmt-print-convert.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert-absl.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert-custom.cpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp
@@ -0,0 +1,1021 @@
+// RUN: %check_clang_tidy -std=c++2b %s modernize-printf-to-std-print %t -- -- -isystem %clang_tidy_headers
+
+#include 
+#include 
+#include 
+
+void printf_simple() {
+  printf("Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("Hello");
+}
+
+void fprintf_simple() {
+  fprintf(stderr, "Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'fprintf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print(stderr, "Hello");
+}
+
+void std_printf_simple() {
+  std::printf("std::Hello");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("std::Hello");
+}
+
+void printf_escape() {
+  printf("before \t");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("before \t");
+
+  printf("\n after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("\n after");
+
+  printf("before \a after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("before \a after");
+
+  printf("Bell\a%dBackspace\bFF%s\fNewline\nCR\rTab\tVT\vEscape\x1b\x07%d", 42, "string", 99);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("Bell\a{}Backspace\bFF{}\fNewline\nCR\rTab\tVT\vEscape\x1b\a{}", 42, "string", 99);
+
+  printf("not special \x1b\x01\x7f");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("not special \x1b\x01\x7f");
+}
+
+void printf_percent() {
+  printf("before %%");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("before %");
+
+  printf("%% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print]
+  // CHECK-FIXES: std::print("% after");
+
+  printf("before %% after");
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print'

44 matches

Mail list logo