Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

README: Add link to merged clang-tidy check #3515

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

mikecrowe
Copy link
Contributor

A much-improved version of the main clang-tidy-fmt check for converting printf and fprintf has landed in upstream LLVM. It converts to std::print by default, but can be configured to convert to fmt::print instead. It makes more sense for the README to point to that version instead now.

A much-improved version of the main clang-tidy-fmt check for converting
printf and fprintf has landed in upstream LLVM. It converts to
std::print by default, but can be configured to convert to fmt::print
instead. It makes more sense for the README to point to that version
instead now.
@mikecrowe
Copy link
Contributor Author

Since you're the world experts on the format specification syntax, it would
be lovely if you could also have a look at the test cases for the check at
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
to see if there are any glaring omissions or mistakes.

Thanks!

@vitaut vitaut merged commit e4c8cfe into fmtlib:master Jun 30, 2023
@vitaut
Copy link
Contributor

vitaut commented Jun 30, 2023

@mikecrowe
Copy link
Contributor Author

In https://github.com/llvm/llvm-project/blob/fc37f717770acdfe5504bb9b969a01bb16a187f9/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp#L339C34-L339C36
why is the format string "{} {{[{][{]}}}}" and not `"{} {{}}"?

LLVM FileCheckexpects anything in {{ and }} to be a regex. So, to match literal {{}} I need to write a regex that matches it and enclose it in those characters. The documentation for FileCheck says:

In the rare case that you want to match double braces explicitly from the input, you can use something ugly like {{[}][}]}} as your pattern.

which is almost what I needed.

Also note that the order of dynamic width/precision and the argument it applies to should be reversed in https://github.com/llvm/llvm-project/blob/fc37f717770acdfe5504bb9b969a01bb16a187f9/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp#L1025 and elsewhere.

Thanks for the review. I shall try to implement that - if I can fathom out how!

@mikecrowe
Copy link
Contributor Author

Also note that the order of dynamic width/precision and the argument it applies to should be reversed in https://github.com/llvm/llvm-project/blob/fc37f717770acdfe5504bb9b969a01bb16a187f9/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp#L1025 and elsewhere.

Hopefully fixed by D154283. Thanks for pointing out the flaw. It was easier to fix than I expected.

PiotrZSL pushed a commit to llvm/llvm-project that referenced this pull request Jul 3, 2023
…print

Victor Zverovich pointed out[1] that printf takes the field width and
precision arguments before the value to be printed whereas std::print
takes the value first (unless positional arguments are used.) Many of
the test cases in use-std-print.cpp were incorrect.

Teach the check to rotate the arguments when required to correct
this. Correct the test cases and add more.

[1] fmtlib/fmt#3515 (comment)

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D154283
veselypeta pushed a commit to veselypeta/cherillvm that referenced this pull request Sep 3, 2024
…print

Victor Zverovich pointed out[1] that printf takes the field width and
precision arguments before the value to be printed whereas std::print
takes the value first (unless positional arguments are used.) Many of
the test cases in use-std-print.cpp were incorrect.

Teach the check to rotate the arguments when required to correct
this. Correct the test cases and add more.

[1] fmtlib/fmt#3515 (comment)

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D154283
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants