-
Notifications
You must be signed in to change notification settings - Fork 15k
[clang-tidy] modernize-use-std-format: Correct replacement types when signed stdint types are used, and when enums are printed in hex. #150343 #155200
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (DaveBrantonCTCT) ChangesThese two examples illustrate the two problems
Will fail to replace correctly. The int8_t will cause this error message, and the replacement will be skipped.
This example will proceed with the replacement, but will fail to add the necessary cast to the enum's underlying type, and so the format string will break your build. That is, the replacement will be
Instead of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests and release notes about your change in clang-tools-extra/docs/ReleaseNotes.rst
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp View the diff from clang-format here.diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
index 76a14c580..104ce5eee 100644
--- a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
+++ b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
@@ -461,32 +461,30 @@ bool FormatStringConverter::emitIntegerArgument(
// In cases where `x` or `X` was specified in the format string
// these will technically have no effect, since the bool can only be zero or
// one. However, it seems best to leave them as-is anyway.
- switch(ArgKind)
- {
- case ConversionSpecifier::Kind::xArg:
- FormatSpec.push_back('x'); // Not strictly needed
- break;
- case ConversionSpecifier::Kind::XArg:
- FormatSpec.push_back('X');
- break;
- default:
- FormatSpec.push_back('d');
+ switch (ArgKind) {
+ case ConversionSpecifier::Kind::xArg:
+ FormatSpec.push_back('x'); // Not strictly needed
+ break;
+ case ConversionSpecifier::Kind::XArg:
+ FormatSpec.push_back('X');
+ break;
+ default:
+ FormatSpec.push_back('d');
}
-
+
} else if (ArgType->isEnumeralType()) {
// If the format string contained `x` or `X`, then use these
// format modifiers. Otherwise the default will work.
- switch(ArgKind)
- {
- case ConversionSpecifier::Kind::xArg:
- FormatSpec.push_back('x');
- break;
- case ConversionSpecifier::Kind::XArg:
- FormatSpec.push_back('X');
- break;
- default:
- break;
+ switch (ArgKind) {
+ case ConversionSpecifier::Kind::xArg:
+ FormatSpec.push_back('x');
+ break;
+ case ConversionSpecifier::Kind::XArg:
+ FormatSpec.push_back('X');
+ break;
+ default:
+ break;
}
// std::format will try to find a specialization to print the enum
@@ -511,24 +509,21 @@ bool FormatStringConverter::emitIntegerArgument(
// Even -Wformat doesn't warn for this. std::format will format as
// unsigned unless we cast it.
if (const std::optional<std::string> MaybeCastType =
- castTypeForArgument(ArgKind, ArgType))
- {
- switch(ArgKind)
- {
- case ConversionSpecifier::Kind::xArg:
- FormatSpec.push_back('x');
- break;
- case ConversionSpecifier::Kind::XArg:
- FormatSpec.push_back('X');
- break;
- default:
- break;
+ castTypeForArgument(ArgKind, ArgType)) {
+ switch (ArgKind) {
+ case ConversionSpecifier::Kind::xArg:
+ FormatSpec.push_back('x');
+ break;
+ case ConversionSpecifier::Kind::XArg:
+ FormatSpec.push_back('X');
+ break;
+ default:
+ break;
}
ArgFixes.emplace_back(
ArgIndex, (Twine("static_cast<") + *MaybeCastType + ">(").str());
- }
- else
+ } else
return conversionNotPossible(
(Twine("argument ") + Twine(ArgIndex) + " cannot be cast to " +
Twine(ArgKind == ConversionSpecifier::Kind::uArg ? "unsigned"
@@ -536,18 +531,16 @@ bool FormatStringConverter::emitIntegerArgument(
" integer type to match format"
" specifier and StrictMode is enabled")
.str());
- } else
- {
- switch(ArgKind)
- {
- case ConversionSpecifier::Kind::xArg:
- FormatSpec.push_back('x');
- break;
- case ConversionSpecifier::Kind::XArg:
- FormatSpec.push_back('X');
- break;
- default:
- if (isRealCharType(ArgType) || !ArgType->isIntegerType()) {
+ } else {
+ switch (ArgKind) {
+ case ConversionSpecifier::Kind::xArg:
+ FormatSpec.push_back('x');
+ break;
+ case ConversionSpecifier::Kind::XArg:
+ FormatSpec.push_back('X');
+ break;
+ default:
+ if (isRealCharType(ArgType) || !ArgType->isIntegerType()) {
// Only specify integer if the argument is of a different type
FormatSpec.push_back('d');
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mention changes in Release Notes.
// In cases where `x` or `X` was specified in the format string | ||
// these will technically have no effect, since the bool can only be zero or | ||
// one. However, it seems best to leave them as-is anyway. | ||
switch(ArgKind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run Clang-Format.
FormatSpec.push_back('X'); | ||
break; | ||
default: | ||
FormatSpec.push_back('d'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FormatSpec.push_back('d'); | |
FormatSpec.push_back('d'); | |
break; |
if (isRealCharType(ArgType) || !ArgType->isIntegerType()) { | ||
// Only specify integer if the argument is of a different type | ||
FormatSpec.push_back('d'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
break; |
…enums are printed in hex.
e37d17a
to
19b4ddf
Compare
llvm/docs/ReleaseNotes.md
Outdated
@@ -150,6 +150,9 @@ Changes to the Debug Info | |||
Changes to the LLVM tools | |||
--------------------------------- | |||
|
|||
* modernize-use-std-format now correctly replaces signed types, and correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such entry should be in clang-tools-extra/docs/ReleaseNotes.rst
. See other entries there as examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, have put this in the right place.
@@ -249,6 +249,10 @@ Changes in existing checks | |||
<clang-tidy/checks/readability/uppercase-literal-suffix>` check to recognize | |||
literal suffixes added in C++23 and C23. | |||
|
|||
- Improved :doc:`modernize-use-std-format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep alphabetcial order (by check name) in this list.
@@ -249,6 +249,10 @@ Changes in existing checks | |||
<clang-tidy/checks/readability/uppercase-literal-suffix>` check to recognize | |||
literal suffixes added in C++23 and C23. | |||
|
|||
- Improved :doc:`modernize-use-std-format | |||
<clang-tidy/checks/modernize/use-std-format>` now correctly replaces signed types, and correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow 80 characters limit.
…ps://github.com/DaveBrantonCTCT/llvm-project into std-format-modernize-fix-enums-and-signed-types
@@ -213,6 +213,11 @@ Changes in existing checks | |||
when the format string is converted to a different type by an implicit | |||
constructor call. | |||
|
|||
- Improved :doc:`modernize-use-std-format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed existing modernize-use-std-format
entry. Convention is to have single entry per check. Just merge existing statement with your text. At least previous release should have such examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write tests for your patch.
These two examples illustrate the two problems
Will fail to replace correctly. The int8_t will cause this error message, and the replacement will be skipped.
Unknown corresponding signed type for non-BuiltinType 'int8_t'
This example will proceed with the replacement, but will fail to add the necessary cast to the enum's underlying type, and so the format string will break your build. That is, the replacement will be
Instead of
Fixes #150343