-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clang-tidy] Enable C99 in implicit-bool-conversion and avoid FP with bool operands in C23
#171070
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
|
@llvm/pr-subscribers-clang-tools-extra Author: mitchell (zeyi2) ChangesCloses #170596 Full diff: https://github.com/llvm/llvm-project/pull/171070.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index c135b426d8608..a5335bdb58c87 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -27,7 +27,9 @@ AST_MATCHER(Stmt, isMacroExpansion) {
return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
}
-AST_MATCHER(Stmt, isC23) { return Finder->getASTContext().getLangOpts().C23; }
+AST_MATCHER(Stmt, isC) {
+ return !Finder->getASTContext().getLangOpts().CPlusPlus;
+}
// Preserve same name as AST_MATCHER(isNULLMacroExpansion)
// NOLINTNEXTLINE(llvm-prefer-static-over-anonymous-namespace)
@@ -306,8 +308,8 @@ void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
hasCastKind(CK_FloatingToBoolean),
hasCastKind(CK_PointerToBoolean),
hasCastKind(CK_MemberPointerToBoolean)),
- // Exclude cases of C23 comparison result.
- unless(allOf(isC23(),
+ // Exclude cases of C comparison result.
+ unless(allOf(isC(),
hasSourceExpression(ignoringParens(
binaryOperator(hasAnyOperatorName(
">", ">=", "==", "!=", "<", "<=")))))),
@@ -350,6 +352,11 @@ void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
unless(hasParent(
binaryOperator(anyOf(BoolComparison, BoolXor,
BoolOpAssignment, BitfieldAssignment)))),
+ // Exclude logical operators in C
+ unless(allOf(isC(), hasParent(binaryOperator(
+ hasAnyOperatorName("&&", "||"),
+ hasLHS(ImplicitCastFromBool),
+ hasRHS(ImplicitCastFromBool))))),
implicitCastExpr().bind("implicitCastFromBool"),
unless(hasParent(BitfieldConstruct)),
// Check also for nested casts, for example: bool -> int -> float.
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
index 101089ccfb2e9..6ae15a9e19fe2 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
@@ -21,7 +21,7 @@ class ImplicitBoolConversionCheck : public ClangTidyCheck {
public:
ImplicitBoolConversionCheck(StringRef Name, ClangTidyContext *Context);
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
- return LangOpts.Bool;
+ return LangOpts.Bool || LangOpts.C99;
}
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 79a768e599cfd..a59f8942ceb8c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -543,7 +543,8 @@ Changes in existing checks
- Improved :doc:`readability-implicit-bool-conversion
<clang-tidy/checks/readability/implicit-bool-conversion>` check by correctly
adding parentheses when the inner expression are implicitly converted
- multiple times.
+ multiple times, enabling the check in C99, and avoiding false positives when
+ using logical operators with ``bool`` operands in C23.
- Improved :doc:`readability-qualified-auto
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c
new file mode 100644
index 0000000000000..dfadbbb5c3a61
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy -std=c99 %s readability-implicit-bool-conversion %t
+
+typedef _Bool bool;
+#define true 1
+#define false 0
+
+bool returns_bool(void) { return true; }
+int returns_int(void) { return 1; }
+
+void test_c99_logical_ops(void) {
+ bool b1 = true;
+ bool b2 = false;
+
+ if (b1 && b2) {}
+
+ if (b1 && returns_int()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
+ // CHECK-FIXES: if ((int)b1 && returns_int()) {}
+}
+
+void test_c99_comparison(void) {
+ int x = 1;
+ int y = 2;
+ bool b = x > y;
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
index 7f82b95f41799..d02f1b84d3dd6 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
@@ -75,20 +75,10 @@ void implicitConversionFromBoolInComplexBoolExpressions() {
bool anotherBoolean = false;
int integer = boolean && anotherBoolean;
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: int integer = (int)boolean && (int)anotherBoolean;
-
float floating = (boolean || anotherBoolean) * 0.3f;
- // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:32: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: float floating = ((int)boolean || (int)anotherBoolean) * 0.3f;
-
double doubleFloating = (boolean && (anotherBoolean || boolean)) * 0.3;
// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:40: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-3]]:58: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: double doubleFloating = ((int)boolean && ((int)anotherBoolean || (int)boolean)) * 0.3;
+ // CHECK-FIXES: double doubleFloating = ((int)boolean && (anotherBoolean || boolean)) * 0.3;
}
void implicitConversionFromBoolLiterals() {
@@ -371,3 +361,28 @@ int keepCompactReturnInC_PR71848() {
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
// CHECK-FIXES: return(int)( foo );
}
+
+bool returns_bool(void) { return true; }
+
+void implicitConversionFromBoolInLogicalOps(int len) {
+ while (returns_bool()) {}
+ while ((len > 0) && returns_bool()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((len > 0) && (int)returns_bool()) {}
+ while (returns_bool() && (len > 0)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((int)returns_bool() && (len > 0)) {}
+ while ((len > 0) || returns_bool()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((len > 0) || (int)returns_bool()) {}
+ while (returns_bool() || (len > 0)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((int)returns_bool() || (len > 0)) {}
+}
+
+void checkResultAssignment(void) {
+ int a = returns_bool() || returns_bool();
+ bool b = returns_bool() && returns_bool();
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicit conversion 'int' -> 'bool'
+ // CHECK-FIXES: bool b = (returns_bool() && returns_bool()) != 0;
+}
|
|
@llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) ChangesCloses #170596 Full diff: https://github.com/llvm/llvm-project/pull/171070.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
index c135b426d8608..a5335bdb58c87 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp
@@ -27,7 +27,9 @@ AST_MATCHER(Stmt, isMacroExpansion) {
return SM.isMacroBodyExpansion(Loc) || SM.isMacroArgExpansion(Loc);
}
-AST_MATCHER(Stmt, isC23) { return Finder->getASTContext().getLangOpts().C23; }
+AST_MATCHER(Stmt, isC) {
+ return !Finder->getASTContext().getLangOpts().CPlusPlus;
+}
// Preserve same name as AST_MATCHER(isNULLMacroExpansion)
// NOLINTNEXTLINE(llvm-prefer-static-over-anonymous-namespace)
@@ -306,8 +308,8 @@ void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
hasCastKind(CK_FloatingToBoolean),
hasCastKind(CK_PointerToBoolean),
hasCastKind(CK_MemberPointerToBoolean)),
- // Exclude cases of C23 comparison result.
- unless(allOf(isC23(),
+ // Exclude cases of C comparison result.
+ unless(allOf(isC(),
hasSourceExpression(ignoringParens(
binaryOperator(hasAnyOperatorName(
">", ">=", "==", "!=", "<", "<=")))))),
@@ -350,6 +352,11 @@ void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
unless(hasParent(
binaryOperator(anyOf(BoolComparison, BoolXor,
BoolOpAssignment, BitfieldAssignment)))),
+ // Exclude logical operators in C
+ unless(allOf(isC(), hasParent(binaryOperator(
+ hasAnyOperatorName("&&", "||"),
+ hasLHS(ImplicitCastFromBool),
+ hasRHS(ImplicitCastFromBool))))),
implicitCastExpr().bind("implicitCastFromBool"),
unless(hasParent(BitfieldConstruct)),
// Check also for nested casts, for example: bool -> int -> float.
diff --git a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
index 101089ccfb2e9..6ae15a9e19fe2 100644
--- a/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h
@@ -21,7 +21,7 @@ class ImplicitBoolConversionCheck : public ClangTidyCheck {
public:
ImplicitBoolConversionCheck(StringRef Name, ClangTidyContext *Context);
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
- return LangOpts.Bool;
+ return LangOpts.Bool || LangOpts.C99;
}
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 79a768e599cfd..a59f8942ceb8c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -543,7 +543,8 @@ Changes in existing checks
- Improved :doc:`readability-implicit-bool-conversion
<clang-tidy/checks/readability/implicit-bool-conversion>` check by correctly
adding parentheses when the inner expression are implicitly converted
- multiple times.
+ multiple times, enabling the check in C99, and avoiding false positives when
+ using logical operators with ``bool`` operands in C23.
- Improved :doc:`readability-qualified-auto
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c
new file mode 100644
index 0000000000000..dfadbbb5c3a61
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion-c99.c
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy -std=c99 %s readability-implicit-bool-conversion %t
+
+typedef _Bool bool;
+#define true 1
+#define false 0
+
+bool returns_bool(void) { return true; }
+int returns_int(void) { return 1; }
+
+void test_c99_logical_ops(void) {
+ bool b1 = true;
+ bool b2 = false;
+
+ if (b1 && b2) {}
+
+ if (b1 && returns_int()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
+ // CHECK-FIXES: if ((int)b1 && returns_int()) {}
+}
+
+void test_c99_comparison(void) {
+ int x = 1;
+ int y = 2;
+ bool b = x > y;
+}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
index 7f82b95f41799..d02f1b84d3dd6 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/implicit-bool-conversion.c
@@ -75,20 +75,10 @@ void implicitConversionFromBoolInComplexBoolExpressions() {
bool anotherBoolean = false;
int integer = boolean && anotherBoolean;
- // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: int integer = (int)boolean && (int)anotherBoolean;
-
float floating = (boolean || anotherBoolean) * 0.3f;
- // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:32: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: float floating = ((int)boolean || (int)anotherBoolean) * 0.3f;
-
double doubleFloating = (boolean && (anotherBoolean || boolean)) * 0.3;
// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-2]]:40: warning: implicit conversion 'bool' -> 'int'
- // CHECK-MESSAGES: :[[@LINE-3]]:58: warning: implicit conversion 'bool' -> 'int'
- // CHECK-FIXES: double doubleFloating = ((int)boolean && ((int)anotherBoolean || (int)boolean)) * 0.3;
+ // CHECK-FIXES: double doubleFloating = ((int)boolean && (anotherBoolean || boolean)) * 0.3;
}
void implicitConversionFromBoolLiterals() {
@@ -371,3 +361,28 @@ int keepCompactReturnInC_PR71848() {
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion 'bool' -> 'int' [readability-implicit-bool-conversion]
// CHECK-FIXES: return(int)( foo );
}
+
+bool returns_bool(void) { return true; }
+
+void implicitConversionFromBoolInLogicalOps(int len) {
+ while (returns_bool()) {}
+ while ((len > 0) && returns_bool()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((len > 0) && (int)returns_bool()) {}
+ while (returns_bool() && (len > 0)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((int)returns_bool() && (len > 0)) {}
+ while ((len > 0) || returns_bool()) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((len > 0) || (int)returns_bool()) {}
+ while (returns_bool() || (len > 0)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'bool' -> 'int'
+ // CHECK-FIXES: while ((int)returns_bool() || (len > 0)) {}
+}
+
+void checkResultAssignment(void) {
+ int a = returns_bool() || returns_bool();
+ bool b = returns_bool() && returns_bool();
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: implicit conversion 'int' -> 'bool'
+ // CHECK-FIXES: bool b = (returns_bool() && returns_bool()) != 0;
+}
|
|
|
||
| AST_MATCHER(Stmt, isC23) { return Finder->getASTContext().getLangOpts().C23; } | ||
| AST_MATCHER(Stmt, isC) { | ||
| return !Finder->getASTContext().getLangOpts().CPlusPlus; |
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.
Will it match Objective-C?
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.
Yes it will. I'll fix it soon. Thanks :)
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.
What about return .getLangOpts().C99? We don't need to handle C89 in this check.
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.
What about
return .getLangOpts().C99? We don't need to handle C89 in this check.
Thanks! Using LangOpts.C99 would indeed exclude C89, but ASAIK in Clang the C-standard flags are cumulative: C11/C17/C23 (and Obj-C, which defaults to gnu11) all set C99 as well. So I think getLO.C99 alone would also match Obj‑C.
And the documentation didn't mention the scope of this check. If we want this check to apply only to "pure" C, there are other LangOpts that need to be excluded as well: return !LO.CPlusPlus && !LO.ObjC && !LO.OpenCL && !LO.CUDA && !LO.HIP && !LO.HLSL;.
| // CHECK-MESSAGES: :[[@LINE-2]]:40: warning: implicit conversion 'bool' -> 'int' | ||
| // CHECK-MESSAGES: :[[@LINE-3]]:58: warning: implicit conversion 'bool' -> 'int' | ||
| // CHECK-FIXES: double doubleFloating = ((int)boolean && ((int)anotherBoolean || (int)boolean)) * 0.3; | ||
| // CHECK-FIXES: double doubleFloating = ((int)boolean && (anotherBoolean || boolean)) * 0.3; |
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.
(anotherBoolean || boolean) returns an int type result, while boolean is bool, so there is an implicit conversion and should get a warning.
Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
Closes #170596