Added options to readability-implicit-bool-conversion #120087
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added new options in ImplicitBoolConversionCheck CheckConversionToBool and CheckConversionFromBool.
Added CheckConversionToBool and CheckConversionFromBool Options in the header
Added new test to check the new options added in the ImplicitBoolConversionCheck.cpp
|
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. |
Member
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: None (4m4n-x-B4w4ne) ChangesAs given in the issue #36323 , I added two new options in the clang-tools-extra/clan-tidy/readibility/ImplicitBoolConversionCheck.cpp and header file. Full diff: https://github.com/llvm/llvm-project/pull/120087.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index f9fd1d903e231e..517a5d2b982751 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -258,14 +258,17 @@ ImplicitBoolConversionCheck::ImplicitBoolConversionCheck(
: ClangTidyCheck(Name, Context),
AllowIntegerConditions(Options.get("AllowIntegerConditions", false)),
AllowPointerConditions(Options.get("AllowPointerConditions", false)),
- UseUpperCaseLiteralSuffix(
- Options.get("UseUpperCaseLiteralSuffix", false)) {}
+ UseUpperCaseLiteralSuffix(Options.get("UseUpperCaseLiteralSuffix", false)),
+ CheckConversionsToBool(Options.get("CheckConversionsToBool",true)),
+ CheckConversionsFromBool(Options.get("CheckConversionsFromBool",true)) {}
void ImplicitBoolConversionCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowIntegerConditions", AllowIntegerConditions);
Options.store(Opts, "AllowPointerConditions", AllowPointerConditions);
Options.store(Opts, "UseUpperCaseLiteralSuffix", UseUpperCaseLiteralSuffix);
+ Options.store(Opts,"CheckConversionsToBool",CheckConversionsToBool);
+ Options.store(Opts,"CheckConversionsFromBool",CheckConversionsFromBool);
}
void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
@@ -358,14 +361,14 @@ void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
void ImplicitBoolConversionCheck::check(
const MatchFinder::MatchResult &Result) {
- if (const auto *CastToBool =
- Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastToBool")) {
+ if (CheckConversionsToBool && (const auto *CastToBool =
+ Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastToBool"))) {
const auto *Parent = Result.Nodes.getNodeAs<Stmt>("parentStmt");
return handleCastToBool(CastToBool, Parent, *Result.Context);
}
- if (const auto *CastFromBool =
- Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastFromBool")) {
+ if (CheckConversionsFromBool && (const auto *CastFromBool =
+ Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastFromBool"))) {
const auto *NextImplicitCast =
Result.Nodes.getNodeAs<ImplicitCastExpr>("furtherImplicitCast");
return handleCastFromBool(CastFromBool, NextImplicitCast, *Result.Context);
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
index 5947f7316e67cc..b0c3c2943e649c 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
@@ -37,6 +37,8 @@ class ImplicitBoolConversionCheck : public ClangTidyCheck {
const bool AllowIntegerConditions;
const bool AllowPointerConditions;
const bool UseUpperCaseLiteralSuffix;
+ const bool CheckConversionsToBool;
+ const bool CheckConversionsFromBool;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-check.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-check.cpp
new file mode 100644
index 00000000000000..506769d5a57322
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-check.cpp
@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t
+
+// RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t \
+// RUN: -- -config='{CheckOptions: [{key: readability-implicit-bool-conversion.CheckConversionsToBool, value: false}, {key: readability-implicit-bool-conversion.CheckConversionsFromBool, value: true}]}'
+
+// RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t \
+// RUN: -- -config='{CheckOptions: [{key: readability-implicit-bool-conversion.CheckConversionsToBool, value: true}, {key: readability-implicit-bool-conversion.CheckConversionsFromBool, value: false}]}'
+
+// RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t \
+// RUN: -- -config='{CheckOptions: [{key: readability-implicit-bool-conversion.CheckConversionsToBool, value: false}, {key: readability-implicit-bool-conversion.CheckConversionsFromBool, value: false}]}'
+
+// ==========================================================
+// Test Case: Conversions to bool (CheckConversionsToBool=true)
+// ==========================================================
+void TestConversionsToBool() {
+ int x = 42;
+ if (x) // CHECK-MESSAGES: :[[@LINE]]:8: warning: implicit conversion 'int' -> 'bool'
+ (void)0;
+
+ float f = 3.14;
+ if (f) // CHECK-MESSAGES: :[[@LINE]]:8: warning: implicit conversion 'float' -> 'bool'
+ (void)0;
+
+ int *p = nullptr;
+ if (p) // CHECK-MESSAGES: :[[@LINE]]:8: warning: implicit conversion 'int *' -> 'bool'
+ (void)0;
+
+ // Pointer-to-member
+ struct S {
+ int member;
+ };
+ int S::*ptr = nullptr;
+ if (ptr) // CHECK-MESSAGES: :[[@LINE]]:8: warning: implicit conversion 'int S::*' -> 'bool'
+ (void)0;
+}
+
+// ==========================================================
+// Test Case: Conversions from bool (CheckConversionsFromBool=true)
+// ==========================================================
+void TestConversionsFromBool() {
+ bool b = true;
+
+ int x = b; // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'int'
+ float f = b; // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'float'
+ int *p = b; // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'int *'
+
+ struct S {
+ int member;
+ };
+ int S::*ptr = b; // CHECK-MESSAGES: :[[@LINE]]:16: warning: implicit conversion 'bool' -> 'int S::*'
+}
+
+// ==========================================================
+// Test Case: Mixed Configurations (ToBool=false, FromBool=true)
+// ==========================================================
+void TestMixedConfig() {
+ int x = 42;
+ if (x) // No warning: CheckConversionsToBool=false
+ (void)0;
+
+ bool b = true;
+ int y = b; // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'int'
+}
+
+// ==========================================================
+// Test Case: No Diagnostics (ToBool=false, FromBool=false)
+// ==========================================================
+void TestNoDiagnostics() {
+ int x = 42;
+ if (x) // No warning: CheckConversionsToBool=false
+ (void)0;
+
+ bool b = true;
+ int y = b; // No warning: CheckConversionsFromBool=false
+}
+
+// ==========================================================
+// Test Case: Edge Cases and Complex Expressions
+// ==========================================================
+void TestEdgeCases() {
+ bool b = true;
+
+ // Nested implicit casts
+ int x = (b ? 1 : 0); // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'int'
+
+ // Function returns implicit bool
+ auto ReturnBool = []() -> bool { return true; };
+ int y = ReturnBool(); // CHECK-MESSAGES: :[[@LINE]]:12: warning: implicit conversion 'bool' -> 'int'
+
+ // Explicit casts (no diagnostics)
+ int z = static_cast<int>(b); // No warning: explicit cast
+}
|
4m4n-x-B4w4ne
changed the title
EugeneZelenko
requested review from
5chmidti,
HerrCai0907,
PiotrZSL and
carlosgalvezp
Contributor
|
Please mention options in Release Notes and check documentation. |
Fixed Bugs
Fixed Bugs in test .
HerrCai0907
reviewed
Dec 17, 2024
Contributor
HerrCai0907
left a comment
HerrCai0907
left a comment
There was a problem hiding this comment.
update release note and doc also
PiotrZSL
requested changes
Dec 17, 2024
Member
PiotrZSL
left a comment
PiotrZSL
left a comment
There was a problem hiding this comment.
- reformat code
- missing release notes entry about new options added
- missing documentation for newly added options
- change should be in registerMatchers method
| // RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t | ||
|
|
||
| // RUN: %check_clang_tidy %s readability-implicit-bool-conversion %t \ | ||
| // RUN: -- -config='{CheckOptions: [{key: readability-implicit-bool-conversion.CheckConversionsToBool, value: false}, {key: readability-implicit-bool-conversion.CheckConversionsFromBool, value: true}]}' |
Member
There was a problem hiding this comment.
this is old format for options, use new one like in other checks
Contributor
Author
There was a problem hiding this comment.
Okay Thanks I am Working on it.
Contributor
Author
There was a problem hiding this comment.
I have done this . Can you please review it?
| void ImplicitBoolConversionCheck::check( | ||
| const MatchFinder::MatchResult &Result) { | ||
|
|
||
| if(CheckConversionsToBool){ |
Member
There was a problem hiding this comment.
Wrong, place these things should be actually put into matcher in registerMatchers method, to speed up.
You got 2 calls to addMatcher, one in line 300, other in line 339, put those (with dependences to avoid unused local variables) into if's.
Contributor
Author
There was a problem hiding this comment.
Okay Thanks I am Working on it.
Contributor
Author
There was a problem hiding this comment.
Can You please check this? I have done this now as instructed.
Added new Options in ImplicitBoolConversionCheck.
Contributor
Author
I am doing it. |
| <clang-tidy/checks/readability/identifier-naming>` check to | ||
| validate ``namespace`` aliases. | ||
|
|
||
| -Improved :doc: `readability-implicit-bool-conversion |
Contributor
There was a problem hiding this comment.
Please extend existing readability-implicit-bool-conversion entry.
|
|
||
| -Improved :doc: `readability-implicit-bool-conversion | ||
| <clang-tidy/checks/readability/implicit-bool-conversion>` | ||
| - `CheckConversionsToBool`: Allows enabling or disabling warnings for implicit conversions to `bool` (default) : true . |
Contributor
There was a problem hiding this comment.
Default belongs to documentation.
| case of the literal suffix in fixes and fixing false positive for implicit | ||
| conversion of comparison result in C23. | ||
| <clang-tidy/checks/readability/implicit-bool-conversion>` | ||
| - `UseUpperCaseLiteralSuffix` : check by adding the option to select the case of the literal suffix in fixes and fixing false positive for implicit conversion of comparison result in C23. |
Contributor
There was a problem hiding this comment.
check by adding the options should be common for all three options.
Contributor
There was a problem hiding this comment.
Sorry, if I was not clear, check by adding the options should be added once before list of added options.
Contributor
Author
There was a problem hiding this comment.
Thanks I have done it in Release Notes .
Contributor
There was a problem hiding this comment.
It is not necessary to repeat check by adding the option thrice.
Contributor
Author
There was a problem hiding this comment.
Its looking good now?
Contributor
|
New options are still not described in check documentation. |
| - `UseUpperCaseLiteralSuffix` : check by adding the option to select the case of the literal suffix in fixes and fixing false positive for implicit conversion of comparison result in C23. | ||
| - `CheckConversionsToBool`: check by adding the option to allow enabling or disabling warnings for implicit conversions to `bool` . | ||
| - `CheckConversionsFromBool`: check by adding the option to allow enabling or disabling warnings for implicit conversions from `bool`. | ||
| Check by adding the option |
Contributor
There was a problem hiding this comment.
Belongs to previous line.
Contributor
Author
There was a problem hiding this comment.
Sorry but i havent got this one . Can you please clarify?
Contributor
Author
There was a problem hiding this comment.
Thanks got it now.
| - `CheckConversionsToBool`: check by adding the option to allow enabling or disabling warnings for implicit conversions to `bool` . | ||
| - `CheckConversionsFromBool`: check by adding the option to allow enabling or disabling warnings for implicit conversions from `bool`. | ||
| Check by adding the option | ||
| - `UseUpperCaseLiteralSuffix` : to select the case of the literal suffix in fixes and fixing false positive for implicit conversion of comparison result in C23. |
Contributor
There was a problem hiding this comment.
Please follow 80-characters limit.
Updated for all permutations of configurations options and also for the more comprehensive tests.
| <clang-tidy/checks/readability/implicit-bool-conversion>` check by adding the | ||
| option `UseUpperCaseLiteralSuffix` to select the case of the literal suffix in | ||
| fixes and fixing false positive for implicit conversion of comparison result in | ||
| C23 , and by adding the option `CheckConversionsToBool` or |
Contributor
There was a problem hiding this comment.
Suggested change
| C23 , and by adding the option `CheckConversionsToBool` or | |
| C23, and by adding the option `CheckConversionsToBool` or |
Contributor
Author
There was a problem hiding this comment.
Hey, What needs to be done further? Can you please help me out?
Contributor
There was a problem hiding this comment.
Please fix Clang-format complains.
HerrCai0907
approved these changes
Dec 27, 2024
Contributor
HerrCai0907
left a comment
HerrCai0907
left a comment
There was a problem hiding this comment.
LGTM. If you do not have write access, please ping me. we can help you to do merge.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
FIxed Clang formatting issue.
Contributor
Author
|
@HerrCai0907 , Thanks I dont have any write permissions. Can you please help me to merge? |
|
@4m4n-x-B4w4ne Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/9909 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/9576 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/198/builds/629 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/7820 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/5676 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/125/builds/4518 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/199/builds/482 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/39/builds/3628 Here is the relevant piece of the build log for the reference |
Closed
chapuni
added a commit
that referenced
this pull request
Dec 28, 2024
This reverts commit 5bec2b7. (llvmorg-20-init-16425-g5bec2b71b44d) This broke tests.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/2535 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/4098 Here is the relevant piece of the build log for the reference |
Contributor
|
Excuse me, I've reverted this. |
Contributor
Author
@chapuni Can you help on what thing , It went wrong and like ,What shall I need to do to make it done? |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/2164 Here is the relevant piece of the build log for the reference |
4m4n-x-B4w4ne
added a commit
to 4m4n-x-B4w4ne/llvm-project
that referenced
this pull request
Jan 24, 2025
|
I'm interested in this configuration. This was reverted after tests broke. What's the status on this now? @chapuni do you have some inputs on how to do test verification before trying to merge this again? |
Contributor
|
@kaddkaka IIRC the issue was "char is unsigned". It could be reproducible with adding default target as failing targets above. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #36323 , I added two new options in the clang-tools-extra/clan-tidy/readibility/ImplicitBoolConversionCheck.cpp and header file.
I have also written new test cases to test these new options in test/readibility directory.