[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 488445.
pfultz2 added a comment.
Merge from main.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A {
+ e1,
+ e2
+};
+
+enum B : unsigned int {
+ e3,
+ e4
+};
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void foo_unsigned(unsigned int i);
+void f1() {
+ foo(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e1));
+}
+void f2() {
+ foo(static_cast(e2));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: bar a(static_cast(e1));
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: auto a = bar{static_cast(e1)};
+}
+int f6() {
+ return e1;
+ // CHECK-NOTES: :[[@LINE-1]]:10: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:10: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: return static_cast(e1);
+}
+void f7() {
+ foo_unsigned(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo_unsigned(static_cast(e1));
+}
+void f8() {
+ foo(e4);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e4));
+}
+void f9() {
+ foo_unsigned(e4);
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo_unsigned(static_cast(e4));
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls, or return statements as such conversions are not always
+clear to the user, but this could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 488440.
pfultz2 added a comment.
Improve null checking, use the correct type in the fixit, and updated the tests.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A {
+ e1,
+ e2
+};
+
+enum B : unsigned int {
+ e3,
+ e4
+};
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void foo_unsigned(unsigned int i);
+void f1() {
+ foo(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e1));
+}
+void f2() {
+ foo(static_cast(e2));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: bar a(static_cast(e1));
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: auto a = bar{static_cast(e1)};
+}
+int f6() {
+ return e1;
+ // CHECK-NOTES: :[[@LINE-1]]:10: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:10: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: return static_cast(e1);
+}
+void f7() {
+ foo_unsigned(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo_unsigned(static_cast(e1));
+}
+void f8() {
+ foo(e4);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e4));
+}
+void f9() {
+ foo_unsigned(e4);
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo_unsigned(static_cast(e4));
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls, or return statements as such conversions are not always
+clear to the user, but this could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index:
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp:45
+ } else {
+Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)");
+ }
carlosgalvezp wrote:
> I don't think we can assume the type of the enum is `int`, see for example:
>
> ```
> enum Foo : unsigned int
> {
> a,
> b
> };
>
>
> void f(unsigned int);
>
> int main()
> {
> f(a);
> }
>
> ```
>
> Even if there is no explicit underlying type, isn't the underlying type
> implementation-defined?
Since its an explicit cast then we should probably use the type that the
function accepts(since thats what it will be eventually converted to) rather
than the type of the underling enum type.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. I would like to see this merged in. Is there anyone available to review or approve? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. @njames93 I fixed the review comments, can this be merged? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. All review comments have been addressed. I assume this can be merged now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. So can this be merged now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. Anymore feedback? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 454632.
pfultz2 added a comment.
Updated to diagnose `return` statements as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,44 @@
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e1));
+}
+void f2() {
+ foo(static_cast(e2));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: bar a(static_cast(e1));
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: auto a = bar{static_cast(e1)};
+}
+int f6() {
+ return e1;
+ // CHECK-NOTES: :[[@LINE-1]]:10: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:10: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: return static_cast(e1);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls, or return statements as such conversions are not always
+clear to the user, but this could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
New check aliases
^
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 453515.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,37 @@
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e1));
+}
+void f2() {
+ foo(static_cast(e2));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: bar a(static_cast(e1));
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: auto a = bar{static_cast(e1)};
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@
Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
New check aliases
^
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Diagnoses
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 453514.
pfultz2 added a comment.
Update review comments and added fixit hints.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,37 @@
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: foo(static_cast(e1));
+}
+void f2() {
+ foo(static_cast(e2));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: bar a(static_cast(e1));
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+ // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast to silence this warning
+ // CHECK-NOTES: static_cast( )
+ // CHECK-FIXES: auto a = bar{static_cast(e1)};
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -99,6 +99,11 @@
New checks
^^
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
New check aliases
^
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. And for context, here is an actual bug this check would help find: https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1578#discussion_r889038610 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. > What is the motivation for requiring these to be the arguments of call or > construct expressions? It is to just to try and limit the possible false positives at first. Usually a function call it is not clear locally that it will be converted to an integer but perhaps its common for users to assign an enum it an integer? I could extend it to support everything except explicit casts if you think that would be better. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 added a comment. Anymore feedback? Can this be merged now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129570/new/ https://reviews.llvm.org/D129570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 451666.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f2() {
+ foo(static_cast(e1));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -99,6 +99,11 @@
New checks
^^
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
New check aliases
^
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/enum-to-int.html
+class EnumToIntCheck : public ClangTidyCheck {
+public:
+ EnumToIntCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif //
[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 updated this revision to Diff 451284.
pfultz2 added a comment.
Fix typo and merge from main.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129570/new/
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f2() {
+ foo(static_cast(e1));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -86,6 +86,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -99,6 +99,11 @@
New checks
^^
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
New check aliases
^
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/enum-to-int.html
+class EnumToIntCheck : public ClangTidyCheck {
+public:
+ EnumToIntCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace bugprone
+} //
[PATCH] D129570: Add new clang-tidy check to find implicit conversions from enum to integer.
pfultz2 created this revision.
pfultz2 added reviewers: aaron.ballman, alexfh.
pfultz2 added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, mgorny.
Herald added a project: All.
pfultz2 requested review of this revision.
Herald added a subscriber: cfe-commits.
This check diagnoses instances where an enum is implicitly converted to an
integer. In C++11, enums can be defined as `enum class` which will prevent
such implicit conversion, however, `enum` provides no such guarantees to
prevent bugs. There can be many reasons why `enum` cannot be replaced with
`enum class` such as compatibility with C or legacy libraries.
This check will diagnose similar implicit conversions when using `enum` to
find the same class of bugs. Currently it will only warn on function or
constructor calls as such conversions are not clear to the user, but this
could be expanded in the future.
void foo(int i);
void f() {
foo(e1); // e1 is implictly converted to an int
}
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D129570
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s bugprone-enum-to-int %t
+
+enum A { e1,
+ e2 };
+
+struct bar {
+ bar(int);
+};
+void foo(int i);
+void f1() {
+ foo(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f2() {
+ foo(static_cast(e1));
+}
+void f3() {
+ int i = e1;
+ foo(i);
+}
+void f4() {
+ bar a(e1);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
+void f5() {
+ auto a = bar{e1};
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Enum is implictly converted to an integral. [bugprone-enum-to-int]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -85,6 +85,7 @@
`bugprone-dangling-handle `_,
`bugprone-dynamic-static-initializers `_,
`bugprone-easily-swappable-parameters `_,
+ `bugprone-enum-to-int `_,
`bugprone-exception-escape `_,
`bugprone-fold-init-type `_,
`bugprone-forward-declaration-namespace `_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantess to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+void foo(int i);
+void f() {
+foo(e1); // e1 is implictly converted to an int
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -118,6 +118,11 @@
New checks
^^
+- New :doc:`bugprone-enum-to-int
+ ` check.
+
+ Finds implicit conversion of enum to an integral type.
+
- New :doc:`bugprone-shared-ptr-array-mismatch ` check.
Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
Index: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.h
@@ -0,0 +1,34 @@
+//===--- EnumToIntCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 added a comment. Thanks for merging as I dont have commit access. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46081/new/ https://reviews.llvm.org/D46081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [CUDA][HIP] Let lambda be host device by default
pfultz2 added inline comments.
Comment at: clang/test/SemaCUDA/lambda.cu:27
+
+kernel<<<1,1>>>([&](){ hd(b); });
+// dev-error@-1 {{capture host side class data member by this pointer in
device or host device lambda function}}
Will this still produce diagnostics when the lambda is explicitly `__device__`?
Maybe you could add a test case for that.
```
kernel<<<1,1>>>([&]() __device__ { hd(b); });
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device
pfultz2 added a comment. > What's the expected HD property of this template function clip? I think it is intended to be host-only. The function `f` will launch a kernel or threads to utilize the passed lambda. Ideally, it would be nice to make all inlineable functions implicitly HD. There is a pragma to force HD, but it is broken(due to forcing HD on functions annotated as host-only or device-only). It would be nice to have a flag to enable such behavior(instead of a pragma). Requiring all these explicit HD annotations just seems like we are moving backwards. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device
pfultz2 added a comment.
> Now, back to the specifics of your example. I'm still not 100% sure I
> understand what the problem is. Can you boil down the use case to an example
> on godbolt?
I dont have a specific example, but there could be code like this generic
`clip` operator:
template
void clip(F f,
const T& min_val,
const T& max_val)
{
f([=](auto x) {
return ::min(::max(min_val, x), max_val);
});
}
Its not clear to the writer of the generic function that it needs to declare
the lambda with an explicit HD.
> If Sam decides to incorporate support for capturing lambdas in this patch, we
> could still do it by restricting the capturing lambda promotion to the ones
> within a function scope only. I.e. lambdas created in global scope would
> still be host.
I think that would be acceptable. I dont think global scope capturing lambdas
are very common due to possible ODR issues.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device
pfultz2 added a comment. > Could you give an example to demonstrate current use and how it will break? Here is place where it would break: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/targets/gpu/device/include/migraphx/gpu/device/multi_index.hpp#L129 This change was already included in a fork of llvm in rocm 3.5 and 3.6 releases which is why this compiles. This also compiles using the hcc-based hip compilers which is what previous rocm versions used. It would be best if this can be upstreamed, so we dont have to hold on to these extra changes in a fork. Part of the motivation for this change was that it wasn't always clear in code where the `__device__` attribute is needed with lambdas sometimes. It also makes it more consistent with `constexpr` lambdas and hcc-based hip compiler. Including this for capturing lambdas will make this simpler and easier to understand. If there are concerns about making it default for capturing lambdas, then can we at least just have a flag to enable this for capturing lambdas? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [CUDA][HIP] Let non-caputuring lambda be host device
pfultz2 added inline comments.
Comment at: clang/lib/Sema/SemaCUDA.cpp:753
return;
+ if (LI.Default == LCD_None && LI.Captures.size() == 0) {
+Method->addAttr(CUDADeviceAttr::CreateImplicit(Context));
There should at least be a flag to enable capturing lambdas to be implicitly
HD. I dont really understand the rational for making capturing lambdas not
implicitly HD. It seems like its trying to prevent using an address to host on
the device, but I dont see how this prevents that at all.
This will also break the compilation in rocm. Should we use a fork of llvm to
compile rocm?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Add -fhip-lambda-host-device
pfultz2 added a comment.
> A slight variation on that, that might be better: lambdas with no
> lambda-capture are implicitly HD; lambdas with any lambda-capture (which must
> therefore have an enclosing function) inherit the enclosing function's HDness.
Lambdas should already inherit the enclosing functions HDness. Keeping
capturing lambdas as implictly HD matches closer the behavior in HIP/HCC, and
as we are porting code it is not always clear which lambdas need explicit HD
annotation since running on the device is an implementation detail.
Capturing lambdas has its pitfalls but they are no different from the same
pitfalls that happen with asynchronous programming or signal callbacks.
Comment at: clang/lib/Sema/SemaCUDA.cpp:733
+ (Method->isConstexpr() ||
+ (getLangOpts().HIPLambdaHostDevice && !LambdaHasRefCaptures(LI {
+Method->addAttr(CUDADeviceAttr::CreateImplicit(Context));
rsmith wrote:
> The reference captures check seems quite strange to me. A copy capture of a
> pointer could have the same problem, as could a copy capture of a class that
> contains a reference or a pointer. As could an init-capture.
>
> These kinds of quirky language rules are usually more trouble than they're
> worth.
Capturing by value is not always an error, only when copying a pointer to a
host variable. but this requires a lot more static analysis to diagnose.
However, capturing by reference is almost always wrong(at least with the
current HIP) when the context is host and the lambda is called on the device.
Therefore, we avoid this scenario by not making such lambdas implicitly HD, but
the error message may not be quite as clear. It is a quirky language rule, and
we could remove this restriction and rely on a warning or static analysis to
diagnose the issue.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Add -fhip-lambda-host-device
pfultz2 added a comment.
Is it possible to add a test like this?
kernel<<<1,1>>>([=](){
auto f = [&]{ hd(); };
f();
});
That should not have a compiler error.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Let lambda be host device by default
pfultz2 added a comment. > It captures addresses as seen at the point in time on the processor which > executed the constructor. Yea and the same happens when assigning the address to a pointer, which is later used on a different device. > Another point that capturing lambdas are not something ready for the prime > time. The same issues exist with functions. We dont prevent passing a pointer to host memory to a device function. I guess because the analysis to do so is incomplete and expensive. A lambda capturing by reference does seem simpler to analyze at least for implicit HD. > Could you elaborate? I'm not sure what you mean by we seem to delay this and > what does it have to do with the assertion that lambdas are not always > constexpr by default? Lambdas are not always `constexpr`, and this patch doesnt make lambdas to always be generated for host and device, even though, it does always have a HD attribute. Instead it pushes the decision to emit the lambda for host or device to later when we are emitting the code for codegen(at least thats how I understand this happening, @yaxunl can correct me if I am wrong here). > I think it's done here: I actually meant how constexpr lambdas was implemented, which I can see here: https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaExpr.cpp#L16087 It doesn't annotate a lambda as `constexpr`. Instead it tries to evaluate all lambdas as `constexpr` when its used in `constexpr` context. This is similar to what we do for HD. We treat all lambdas as HD, but then only emit it for the device when its called in a device function. The big difference is that `constexpr` doesn't participate in overload resolution so constexpr lambdas are not observable by the user whereas host and device attributes are. > We basically slap implicit HD on constexpr functions when we process function > declarations. It's likely that lambdas may go through a different code path > and miss this. Yea, which wont work for lambdas since `NewD->isConstexpr()` will return false(unless the user explicitly adds `constexpr`). We could traverse the AST to see if the lambda only calls constexpr functions and then annotate it with HD(we could also extend this to HD functions as well). However, this seems costly. It would be better to take the same approach for `constexpr` lambdas and treat all lambdas as potentially HD(which is what this patch seems to do). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Let lambda be host device by default
pfultz2 added a comment.
> I.e. if I pass a mutable lambda by reference to the GPU kernel
I dont think we are enabling passing host objects by reference through
functions. Although it could be possible to capture the mutable lambda by
reference by another lambda.
> will the same lambda called on host do the same thing when it's called on the
> device?
Yes, just as the same as capturing a host variable by reference and using it on
the device.
> In principle it would work if GPU and host operate un a uniform memory
A unified memory is not necessary. What is needed is a coordination between the
compiler and runtime.
We dont support capturing host variable by reference, so maybe we can restrict
the implicit HD to lambdas that don't capture by reference?
> According to cppreference, it's only true since C++17 and, AFAICT, only for
> capture-less lambdas.
You can capture as well, if its in a `constexpr` context.
> Considering they are not always constexpr, this assertion is not true, either.
Yes, we seem to delay this. It is always HD but not always emitted for both
host and device.
The issue would be if users tried to detect HD using SFINAE. It could be a
false claim, but maybe it doesnt matter. More importantly, if the lambda is
called in a unevaluated context, will the compiler still emit the function or
will it produce a hard error instead of a substitution failure? I assume
something like this would compile:
template
__host__ auto is_host(F f) -> decltype(f(), std::true_type{});
std::false_type is_host(...);
template
__device__ auto is_device(F f) -> decltype(f(), std::true_type{});
std::false_type is_device(...);
__host__ void f();
void g()
{
auto l = []{ f(); };
using on_host = decltype(is_host(l));
static_assert(on_host{}, "Lambda not on host");
using on_device = decltype(is_device(l));
static_assert(on_device{}, "Lambda not on device");
}
> If/when operator() does get constexpr treatment by compiler, we should
> already derive HD attributes from constexpr. If we do not, then that's what
> needs to be fixed.
How does the compiler implement this? Does it add `constexpr` attribute onto
the operator() or does the constexpr-evalutation visits the lambda as if it
were `constexpr`? It seems the latter would be more effecient, and it would be
similar to what we are doing with HD. The only difference is that a function
can be overloaded with `__host__` and `__device__` whereas that is not possible
with `constexpr`. So a difference could be detected by the user, but maybe that
doesn't matter
> That at least would make sense from consistency standpoint as we currently do
> treat all other constexpr functions as HD.
I mean consistent across the different attributes not in the interpretation of
constexpr. A lambda that only calls constexpr functions implicitly has
`constexpr` attribute. So, a lambda that only calls device functions(or HD)
should implicitly have the `__device__` attribute.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Let lambda be host device by default
pfultz2 added a comment. > Not only the capture is an issue, like a regular function, lambda could also > access non-local variables/functions. In practice this is not an issue. Hcc will implictly treat anything inlinable as host device, and user's are not confused or surprised when they use non-local variables/reference that are on the host. > From the other perspective, a lambda is just another function and should have > consistent rule for its usage, resolution, and etc. But its not like another function. It has internal linkage(even when using global variables declared with `inline`). Lambdas are also implicitly `constexpr` whereas a function need to explicitly declare `constexpr`. Making lambdas implicitly HD whereas function need to be explicit seems to be consistent with how lambdas work with `constexpr`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Let lambda be host device by default
pfultz2 added a subscriber: AlexVlx. pfultz2 added a comment. > says we capture host variable reference in a device lambda. Is that required to be an error? I know @AlexVlx added support to hcc at one point to capture host variables by reference. So it seems to be possible for it to work correctly. So it doesn't seem to be like reason enough to disallow implicit HD. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D78655: [HIP] Let lambda be host device by default
pfultz2 added inline comments.
Comment at: clang/lib/Sema/SemaCUDA.cpp:723
+Method->addAttr(CUDADeviceAttr::CreateImplicit(Context));
+Method->addAttr(CUDAHostAttr::CreateImplicit(Context));
+return;
Shouldn't we add these attributes if there are no host and device attributes?
This seems like it will treat `[]() __device__ {}` as host device.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78655/new/
https://reviews.llvm.org/D78655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code
pfultz2 added a comment. This needs a test when calling in a `constexpr` function. I believe `std::invoke` is not `constepxr`, so a function object call in a `constexpr` function should not suggest `std::invoke`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52281 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 updated this revision to Diff 146719.
pfultz2 added a comment.
Rebased
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-allow-enabling-analyzer-alpha-checkers' is visible for users.
+// RUN: clang-tidy -help | not grep 'allow-enabling-analyzer-alpha-checkers'
+
+// Check if '-allow-enabling-analyzer-alpha-checkers' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -allow-enabling-analyzer-alpha-checkers | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -181,6 +181,14 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option allows enabling the experimental alpha checkers from the static
+/// analyzer. This option is set to false and not visible in help, because it is
+/// highly not recommended for users.
+static cl::opt
+AllowEnablingAnalyzerAlphaCheckers("allow-enabling-analyzer-alpha-checkers",
+ cl::init(false), cl::Hidden,
+ cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -335,7 +343,8 @@
<< EC.message() << "\n";
}
ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
- std::vector EnabledChecks = getCheckNames(EffectiveOptions);
+ std::vector EnabledChecks =
+ getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
if (ExplainConfig) {
// FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
@@ -366,7 +375,8 @@
}
if (DumpConfig) {
-EffectiveOptions.CheckOptions = getCheckOptions(EffectiveOptions);
+EffectiveOptions.CheckOptions =
+getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
llvm::outs() << configurationAsText(
ClangTidyOptions::getDefaults().mergeWith(
EffectiveOptions))
@@ -390,7 +400,8 @@
llvm::InitializeAllTargetMCs();
llvm::InitializeAllAsmParsers();
- ClangTidyContext Context(std::move(OwningOptionsProvider));
+ ClangTidyContext Context(std::move(OwningOptionsProvider),
+ AllowEnablingAnalyzerAlphaCheckers);
runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
EnableCheckProfile);
ArrayRef Errors = Context.getErrors();
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -99,7 +99,8 @@
class ClangTidyContext {
public:
/// \brief Initializes \c ClangTidyContext instance.
- ClangTidyContext(std::unique_ptr OptionsProvider);
+ ClangTidyContext(std::unique_ptr OptionsProvider,
+ bool AllowEnablingAnalyzerAlphaCheckers = false);
~ClangTidyContext();
@@ -178,6 +179,12 @@
return CurrentBuildDirectory;
}
+ /// \brief If the experimental alpha checkers from the static analyzer can be
+ /// enabled.
+ bool canEnableAnalyzerAlphaCheckers() const {
+return AllowEnablingAnalyzerAlphaCheckers;
+ }
+
private:
// Calls setDiagnosticsEngine() and storeError().
friend class ClangTidyDiagnosticConsumer;
@@ -209,6 +216,8 @@
llvm::DenseMap CheckNamesByDiagnosticID;
bool Profile;
+
+ bool AllowEnablingAnalyzerAlphaCheckers;
};
/// \brief A diagnostic consumer that turns each \c Diagnostic into a
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -177,9 +177,11 @@
};
ClangTidyContext::ClangTidyContext(
-std::unique_ptr OptionsProvider)
+std::unique_ptr OptionsProvider,
+bool AllowEnablingAnalyzerAlphaCheckers)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
- Profile(false) {
+ Profile(false),
+ AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
// Before the first translation unit we can get errors related to command-line
// parsing, use empty string for the file name in this case.
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. Is someone able to merge in my changes? https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 updated this revision to Diff 145925.
pfultz2 added a comment.
Some changes based on feedback.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-allow-enabling-analyzer-alpha-checkers' is visible for users.
+// RUN: clang-tidy -help | not grep 'allow-enabling-analyzer-alpha-checkers'
+
+// Check if '-allow-enabling-analyzer-alpha-checkers' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -allow-enabling-analyzer-alpha-checkers | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -192,6 +192,14 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option allows enabling the experimental alpha checkers from the static
+/// analyzer. This option is set to false and not visible in help, because it is
+/// highly not recommended for users.
+static cl::opt
+AllowEnablingAnalyzerAlphaCheckers("allow-enabling-analyzer-alpha-checkers",
+ cl::init(false), cl::Hidden,
+ cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -388,7 +396,8 @@
<< EC.message() << "\n";
}
ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
- std::vector EnabledChecks = getCheckNames(EffectiveOptions);
+ std::vector EnabledChecks =
+ getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
if (ExplainConfig) {
// FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
@@ -419,7 +428,8 @@
}
if (DumpConfig) {
-EffectiveOptions.CheckOptions = getCheckOptions(EffectiveOptions);
+EffectiveOptions.CheckOptions =
+getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
llvm::outs() << configurationAsText(
ClangTidyOptions::getDefaults().mergeWith(
EffectiveOptions))
@@ -444,7 +454,8 @@
llvm::InitializeAllTargetMCs();
llvm::InitializeAllAsmParsers();
- ClangTidyContext Context(std::move(OwningOptionsProvider));
+ ClangTidyContext Context(std::move(OwningOptionsProvider),
+ AllowEnablingAnalyzerAlphaCheckers);
runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
EnableCheckProfile ? : nullptr);
ArrayRef Errors = Context.getErrors();
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -104,7 +104,8 @@
class ClangTidyContext {
public:
/// \brief Initializes \c ClangTidyContext instance.
- ClangTidyContext(std::unique_ptr OptionsProvider);
+ ClangTidyContext(std::unique_ptr OptionsProvider,
+ bool AllowEnablingAnalyzerAlphaCheckers = false);
~ClangTidyContext();
@@ -186,6 +187,12 @@
return CurrentBuildDirectory;
}
+ /// \brief If the experimental alpha checkers from the static analyzer can be
+ /// enabled.
+ bool canEnableAnalyzerAlphaCheckers() const {
+return AllowEnablingAnalyzerAlphaCheckers;
+ }
+
private:
// Calls setDiagnosticsEngine() and storeError().
friend class ClangTidyDiagnosticConsumer;
@@ -217,6 +224,8 @@
llvm::DenseMap CheckNamesByDiagnosticID;
ProfileData *Profile;
+
+ bool AllowEnablingAnalyzerAlphaCheckers;
};
/// \brief A diagnostic consumer that turns each \c Diagnostic into a
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -177,9 +177,11 @@
};
ClangTidyContext::ClangTidyContext(
-std::unique_ptr OptionsProvider)
+std::unique_ptr OptionsProvider,
+bool AllowEnablingAnalyzerAlphaCheckers)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
- Profile(nullptr) {
+ Profile(nullptr),
+ AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
// Before the first translation unit we can get errors related to command-line
// parsing,
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > In this sense bug reports against abandoned alpha checkers (which are, > unfortunately, the majority) aren't very useful. In most cases it's just too > easy to see how broken they are. Although the majority are like that, not all of them are. Some like the Conversion checker doesn't seem to have any issues(because its fairly limited at this point). Others like StreamChecker probably just needs some simple fixes. Maybe we can move the alpha checks that have lots of issues and no contributions for awhile to another category such as abandoned. > But if you are interested in a particular checker and want to work on it to > make sure it's stable, we'd be glad to help, so please contact us on the > mailing list. I am definitely interested in working on getting some of the checkers stable, and have already started some work on expanding the ConversionChecker, but would like user feedback as I move forward. https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 updated this revision to Diff 145229.
pfultz2 added a comment.
Moved flag for alpha checks to the ClangTidyContext
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-allow-enabling-analyzer-alpha-checkers' is visible for users
+// RUN: clang-tidy -help | not grep 'allow-enabling-analyzer-alpha-checkers'
+
+// Check if '-allow-enabling-analyzer-alpha-checkers' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -allow-enabling-analyzer-alpha-checkers | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -192,6 +192,14 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option allows enabling alpha checkers from the static analyzer, that
+/// are experimental. This option is set to false and not visible in help,
+/// because it is highly not recommended for users.
+static cl::opt
+AllowEnablingAnalyzerAlphaCheckers("allow-enabling-analyzer-alpha-checkers",
+ cl::init(false), cl::Hidden,
+ cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -388,7 +396,8 @@
<< EC.message() << "\n";
}
ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
- std::vector EnabledChecks = getCheckNames(EffectiveOptions);
+ std::vector EnabledChecks =
+ getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
if (ExplainConfig) {
// FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
@@ -419,7 +428,8 @@
}
if (DumpConfig) {
-EffectiveOptions.CheckOptions = getCheckOptions(EffectiveOptions);
+EffectiveOptions.CheckOptions =
+getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
llvm::outs() << configurationAsText(
ClangTidyOptions::getDefaults().mergeWith(
EffectiveOptions))
@@ -444,7 +454,8 @@
llvm::InitializeAllTargetMCs();
llvm::InitializeAllAsmParsers();
- ClangTidyContext Context(std::move(OwningOptionsProvider));
+ ClangTidyContext Context(std::move(OwningOptionsProvider),
+ AllowEnablingAnalyzerAlphaCheckers);
runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
EnableCheckProfile ? : nullptr);
ArrayRef Errors = Context.getErrors();
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -104,7 +104,8 @@
class ClangTidyContext {
public:
/// \brief Initializes \c ClangTidyContext instance.
- ClangTidyContext(std::unique_ptr OptionsProvider);
+ ClangTidyContext(std::unique_ptr OptionsProvider,
+ bool AllowEnablingAnalyzerAlphaCheckers = false);
~ClangTidyContext();
@@ -186,6 +187,11 @@
return CurrentBuildDirectory;
}
+ /// \brief Turns on experimental alpha checkers from the static analyzer.
+ bool isAlphaChecksAllowed() const {
+return AllowEnablingAnalyzerAlphaCheckers;
+ }
+
private:
// Calls setDiagnosticsEngine() and storeError().
friend class ClangTidyDiagnosticConsumer;
@@ -217,6 +223,8 @@
llvm::DenseMap CheckNamesByDiagnosticID;
ProfileData *Profile;
+
+ bool AllowEnablingAnalyzerAlphaCheckers;
};
/// \brief A diagnostic consumer that turns each \c Diagnostic into a
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -177,9 +177,11 @@
};
ClangTidyContext::ClangTidyContext(
-std::unique_ptr OptionsProvider)
+std::unique_ptr OptionsProvider,
+bool AllowEnablingAnalyzerAlphaCheckers)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
- Profile(nullptr) {
+ Profile(nullptr),
+ AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
// Before the first translation unit we can get errors related to command-line
// parsing, use
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. There doesn't seem to be a simple way to remove it from the ClangTidyOptions class, as a lot more functions need to be changed to support that. I would prefer to leave it there for now, and we can refactor it later. Either way, the check can't be set from a config file. https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 updated this revision to Diff 145072.
pfultz2 added a comment.
Rename flag to `AllowEnablingAnalyzerAlphaCheckers`.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-allow-enabling-analyzer-alpha-checkers' is visible for users
+// RUN: clang-tidy -help | not grep 'allow-enabling-analyzer-alpha-checkers'
+
+// Check if '-allow-enabling-analyzer-alpha-checkers' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -allow-enabling-analyzer-alpha-checkers | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -192,6 +192,14 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option allows enabling alpha checkers from the static analyzer, that
+/// are experimental. This option is set to false and not visible in help,
+/// because it is highly not recommended for users.
+static cl::opt
+AllowEnablingAnalyzerAlphaCheckers("allow-enabling-analyzer-alpha-checkers",
+ cl::init(false), cl::Hidden,
+ cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -301,6 +309,8 @@
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
+ DefaultOptions.AllowEnablingAnalyzerAlphaCheckers =
+ AllowEnablingAnalyzerAlphaCheckers;
DefaultOptions.FormatStyle = FormatStyle;
DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
// USERNAME is used on Windows.
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -77,6 +77,9 @@
/// \brief Turns on temporary destructor-based analysis.
llvm::Optional AnalyzeTemporaryDtors;
+ /// \brief Turns on experimental alpha checkers from the static analyzer.
+ llvm::Optional AllowEnablingAnalyzerAlphaCheckers;
+
/// \brief Format code around applied fixes with clang-format using this
/// style.
///
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -108,6 +108,7 @@
Options.HeaderFilterRegex = "";
Options.SystemHeaders = false;
Options.AnalyzeTemporaryDtors = false;
+ Options.AllowEnablingAnalyzerAlphaCheckers = false;
Options.FormatStyle = "none";
Options.User = llvm::None;
for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
@@ -148,6 +149,8 @@
overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
overrideValue(Result.SystemHeaders, Other.SystemHeaders);
overrideValue(Result.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors);
+ overrideValue(Result.AllowEnablingAnalyzerAlphaCheckers,
+Other.AllowEnablingAnalyzerAlphaCheckers);
overrideValue(Result.FormatStyle, Other.FormatStyle);
overrideValue(Result.User, Other.User);
mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -303,11 +303,12 @@
typedef std::vector> CheckersList;
-static CheckersList getCheckersControlList(ClangTidyContext ) {
+static CheckersList getCheckersControlList(ClangTidyContext ,
+ bool IncludeExperimental) {
CheckersList List;
const auto =
- AnalyzerOptions::getRegisteredCheckers(/*IncludeExperimental=*/false);
+ AnalyzerOptions::getRegisteredCheckers(IncludeExperimental);
bool AnalyzerChecksEnabled = false;
for (StringRef CheckName : RegisteredCheckers) {
std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
@@ -374,7 +375,8 @@
AnalyzerOptions->Config["cfg-temporary-dtors"] =
Context.getOptions().AnalyzeTemporaryDtors ? "true" : "false";
- AnalyzerOptions->CheckersControlList = getCheckersControlList(Context);
+ AnalyzerOptions->CheckersControlList = getCheckersControlList(
+ Context,
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added inline comments. Comment at: clang-tidy/ClangTidyOptions.h:80-82 + /// \brief Turns on experimental alpha checkers from the static analyzer. + llvm::Optional AllowEnablingAlphaChecks; + alexfh wrote: > Since this will only be configurable via a flag, this option will be global > (i.e. not depend on the location of the file being analyzed). I'd suggest to > remove this option altogether and use something else to pass it to > ClangTidyASTConsumerFactory. It could be stashed into > ClangTidyASTConsumerFactory and passed as a parameter of runClangTidy, or it > could live in ClangTidyContext. But it needs to be passed to `getCheckNames` as well, which doesn't take a `ClangTidyContext`. https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > When something is merged into Clang trunk, the expectation is that it will be > production quality or will be worked on rapidly to get it to production > quality, which is somewhat orthogonal to getting user feedback. I don't know > that I have the full history of all of the alpha checks so my generalization > may be inaccurate, but it seems like some of these checks were accepted as a > WIP and the "progress" stopped for a long time with no one volunteering to > remove the features from the analyzer. Some checkers work better than others, while some just need some simple fixes. Of course, no one will volunteer to fix or remove them if they aren't available to run. > That does not require clang-tidy to surface the alpha checks, correct? A good portion of users are using clang-tidy to run the static analyzer, and I would like it to be exposed to them. https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 updated this revision to Diff 145029.
pfultz2 added a comment.
Rename flag to `allow-enabling-alpha-checks` and removed the option from the
clang-tidy file.
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-allow-enabling-alpha-checks' is visible for users
+// RUN: clang-tidy -help | not grep 'enable-alpha-checks'
+
+// Check if '-allow-enabling-alpha-checks' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -allow-enabling-alpha-checks | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -192,6 +192,13 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option allows enabling alpha checkers from the static analyzer, that
+/// are experimental. This option is set to false and not visible in help,
+/// because it is highly not recommended for users.
+static cl::opt AllowEnablingAlphaChecks("allow-enabling-alpha-checks",
+ cl::init(false), cl::Hidden,
+ cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -301,6 +308,7 @@
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
+ DefaultOptions.AllowEnablingAlphaChecks = AllowEnablingAlphaChecks;
DefaultOptions.FormatStyle = FormatStyle;
DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
// USERNAME is used on Windows.
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -77,6 +77,9 @@
/// \brief Turns on temporary destructor-based analysis.
llvm::Optional AnalyzeTemporaryDtors;
+ /// \brief Turns on experimental alpha checkers from the static analyzer.
+ llvm::Optional AllowEnablingAlphaChecks;
+
/// \brief Format code around applied fixes with clang-format using this
/// style.
///
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -108,6 +108,7 @@
Options.HeaderFilterRegex = "";
Options.SystemHeaders = false;
Options.AnalyzeTemporaryDtors = false;
+ Options.AllowEnablingAlphaChecks = false;
Options.FormatStyle = "none";
Options.User = llvm::None;
for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
@@ -148,6 +149,8 @@
overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
overrideValue(Result.SystemHeaders, Other.SystemHeaders);
overrideValue(Result.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors);
+ overrideValue(Result.AllowEnablingAlphaChecks,
+Other.AllowEnablingAlphaChecks);
overrideValue(Result.FormatStyle, Other.FormatStyle);
overrideValue(Result.User, Other.User);
mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -303,11 +303,12 @@
typedef std::vector> CheckersList;
-static CheckersList getCheckersControlList(ClangTidyContext ) {
+static CheckersList getCheckersControlList(ClangTidyContext ,
+ bool IncludeExperimental) {
CheckersList List;
const auto =
- AnalyzerOptions::getRegisteredCheckers(/*IncludeExperimental=*/false);
+ AnalyzerOptions::getRegisteredCheckers(IncludeExperimental);
bool AnalyzerChecksEnabled = false;
for (StringRef CheckName : RegisteredCheckers) {
std::string ClangTidyCheckName((AnalyzerCheckNamePrefix + CheckName).str());
@@ -374,7 +375,8 @@
AnalyzerOptions->Config["cfg-temporary-dtors"] =
Context.getOptions().AnalyzeTemporaryDtors ? "true" : "false";
- AnalyzerOptions->CheckersControlList = getCheckersControlList(Context);
+ AnalyzerOptions->CheckersControlList = getCheckersControlList(
+ Context, *Context.getOptions().AllowEnablingAlphaChecks);
if (!AnalyzerOptions->CheckersControlList.empty()) {
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > I think the premise is a bit off the mark. It's not that these are not for > the common user -- it's that they're simply not ready for users at all. Then why was it merged into clang in the first place? It seems like the whole point of merging it into clang is to get user feedback. Furthermore, I am trying to update the conversion checker to report more errors, and I would like it to be exposed to users so I can find or fix the FPs. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > But still, could you explain the use case and why a local modification of > clang-tidy is not an option? Because I would like to direct users to try an alpha check on thier local codebases without having to tell them to rebuild clang. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added inline comments. Comment at: clang-tidy/ClangTidy.cpp:373-376 // FIXME: Remove this option once clang's cfg-temporary-dtors option defaults // to true. AnalyzerOptions->Config["cfg-temporary-dtors"] = Context.getOptions().AnalyzeTemporaryDtors ? "true" : "false"; alexfh wrote: > alexfh wrote: > > NoQ wrote: > > > Btw this is already enabled by default. > > Should we kill the flag completely? > I've removed the clang-tidy configuration option in r331456. I didnt modify this line of code. Are you just wanting me to rebase? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > As Devin says (and as we discussed this with Anna Zaks) alpha checkers are > still in development, so we don't want to expose them to the users, even very > curious ones. Then why do we make them available with `clang --analyze`? If the plan is not to expose them to the users at all, they should be removed from the codebase, as they are just sitting there bit-rotting. Ideally, I see no problem exposing them to users. This will allow more users to run them on their codebases and submit issues or patches for the problems they find. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > Do you have some better choices? I could do `-allow-alpha-checks`. What do you think? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. Do you want the flag to be renamed? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > Am i understanding correctly that the new -enable-alpha-checks flag doesn't > enable alpha checks, but it only allows enabling them with a separate command? Yes, it enables them to be selected by clang tidy. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 added a comment. > I'm curious about the use case for this, since I don't think it ever makes > sense to run all the alpha checks at once. These checks are typically a work > in progress (they're not finished yet) and often have false positives that > haven't been fixed yet. We want to run several of the alpha checks in our codebase, and as we find bugs and FPs we can then contribute patches to fix them. Since most of our repos use compile_commands.json, it easier to do this in clang tidy instead of scan-build or clang driver. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46159 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers
pfultz2 created this revision.
pfultz2 added reviewers: aaron.ballman, hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, xazax.hun.
The alpha checkers can already be enabled using the clang driver, this allows
them to be enabled using the clang-tidy as well. This can make it easier to
test the alpha checkers with projects which already support the
compile_commands.json. It will also allow more people to give feedback and
patches about the alpha checkers since they can run it as part of clang tidy
checks.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46159
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/enable-alpha-checks.cpp
Index: test/clang-tidy/enable-alpha-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/enable-alpha-checks.cpp
@@ -0,0 +1,6 @@
+// Check if '-enable-alpha-checks' is visible for users
+// RUN: clang-tidy -help | not grep 'enable-alpha-checks'
+
+// Check if '-enable-alpha-checks' enables alpha checks.
+// RUN: clang-tidy -checks=* -list-checks | not grep 'clang-analyzer-alpha'
+// RUN: clang-tidy -checks=* -list-checks -enable-alpha-checks | grep 'clang-analyzer-alpha'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -192,6 +192,12 @@
cl::init(false),
cl::cat(ClangTidyCategory));
+/// This option Enables alpha checkers from the static analyzer, that are
+/// experimental. This option is set to false and not visible in help, because
+/// it is highly not recommended for users.
+static cl::opt EnableAlphaChecks("enable-alpha-checks", cl::init(false),
+ cl::Hidden, cl::cat(ClangTidyCategory));
+
static cl::opt ExportFixes("export-fixes", cl::desc(R"(
YAML file to store suggested fixes in. The
stored fixes can be applied to the input source
@@ -301,6 +307,7 @@
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
+ DefaultOptions.EnableAlphaChecks = EnableAlphaChecks;
DefaultOptions.FormatStyle = FormatStyle;
DefaultOptions.User = llvm::sys::Process::GetEnv("USER");
// USERNAME is used on Windows.
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -77,6 +77,9 @@
/// \brief Turns on temporary destructor-based analysis.
llvm::Optional AnalyzeTemporaryDtors;
+ /// \brief Turns on experimental alpha checkers from the static analyzer.
+ llvm::Optional EnableAlphaChecks;
+
/// \brief Format code around applied fixes with clang-format using this
/// style.
///
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -87,6 +87,7 @@
IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
+IO.mapOptional("EnableAlphaChecks", Options.EnableAlphaChecks);
IO.mapOptional("FormatStyle", Options.FormatStyle);
IO.mapOptional("User", Options.User);
IO.mapOptional("CheckOptions", NOpts->Options);
@@ -108,6 +109,7 @@
Options.HeaderFilterRegex = "";
Options.SystemHeaders = false;
Options.AnalyzeTemporaryDtors = false;
+ Options.EnableAlphaChecks = false;
Options.FormatStyle = "none";
Options.User = llvm::None;
for (ClangTidyModuleRegistry::iterator I = ClangTidyModuleRegistry::begin(),
@@ -148,6 +150,7 @@
overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
overrideValue(Result.SystemHeaders, Other.SystemHeaders);
overrideValue(Result.AnalyzeTemporaryDtors, Other.AnalyzeTemporaryDtors);
+ overrideValue(Result.EnableAlphaChecks, Other.EnableAlphaChecks);
overrideValue(Result.FormatStyle, Other.FormatStyle);
overrideValue(Result.User, Other.User);
mergeVectors(Result.ExtraArgs, Other.ExtraArgs);
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -303,11 +303,12 @@
typedef std::vector> CheckersList;
-static CheckersList getCheckersControlList(ClangTidyContext ) {
+static CheckersList getCheckersControlList(ClangTidyContext ,
+ bool IncludeExperimental) {
CheckersList List;
const auto =
-
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 updated this revision to Diff 144202.
pfultz2 added a comment.
So I ran this on clang/llvm code base and fixed some false positives.
https://reviews.llvm.org/D46081
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
test/Analysis/conversion.cpp
Index: test/Analysis/conversion.cpp
===
--- /dev/null
+++ test/Analysis/conversion.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -Wno-conversion -Wno-tautological-constant-compare -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+// expected-no-diagnostics
+
+void dontwarn1() {
+ unsigned long x = static_cast(-1);
+}
+
+void dontwarn2(unsigned x) {
+ if (x == static_cast(-1)) {
+ }
+}
+
+struct C {
+ C(unsigned x, unsigned long y) {}
+};
+
+void f(C) {}
+
+void functioncall1(long x) {
+ f(C(64, x));
+}
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -102,6 +102,23 @@
S = U / S; // expected-warning {{Loss of sign}}
}
+void f(unsigned x) {}
+void g(unsigned x) {}
+
+void functioncall1() {
+ long x = -1;
+ int y = 0;
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+}
+
+void functioncall2(int x, int y) {
+ if (x < 0)
+f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+}
+
void dontwarn1(unsigned U, signed S) {
U8 = S; // It might be known that S is always 0x00-0xff.
S8 = U; // It might be known that U is always 0x00-0xff.
@@ -129,14 +146,36 @@
DOSTUFF;
}
-// don't warn for calculations
-// seen some fp. For instance: c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
-// there is a todo in the checker to handle calculations
void dontwarn5() {
- signed S = -32;
- U8 = S + 10;
+ unsigned char c1 = 'A';
+ c1 = (c1 >= 'A' && c1 <= 'Z') ? c1 - 'A' + 'a' : c1;
+ unsigned char c2 = 0;
+ c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+ unsigned char c3 = 'Z';
+ c3 = (c3 >= 'A' && c3 <= 'Z') ? c3 - 'A' + 'a' : c3;
+ unsigned char c4 = 'a';
+ c4 = (c4 >= 'A' && c4 <= 'Z') ? c4 - 'A' + 'a' : c4;
+ unsigned char c5 = '@';
+ c5 = (c5 >= 'A' && c5 <= 'Z') ? c5 - 'A' + 'a' : c5;
+}
+
+void dontwarn6() {
+ int x = ~0;
+ unsigned y = ~0;
}
+void dontwarn7(unsigned x) {
+ if (x == (unsigned)-1) {
+ }
+}
+
+void dontwarn8() {
+ unsigned x = (unsigned)-1;
+}
+
+unsigned dontwarn9() {
+ return ~0;
+}
// false positives..
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -53,9 +53,8 @@
void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
CheckerContext ) const {
- // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
- // calculations also.
- if (!isa(Cast->IgnoreParenImpCasts()))
+ // Don't warn for implicit conversions to bool
+ if (Cast->getType()->isBooleanType())
return;
// Don't warn for loss of sign/precision in macros.
@@ -67,16 +66,21 @@
const Stmt *Parent = PM.getParent(Cast);
if (!Parent)
return;
+ // Dont warn if this is part of an explicit cast
+ if (isa(Parent))
+return;
bool LossOfSign = false;
bool LossOfPrecision = false;
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
if (Opc == BO_Assign) {
- LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+ if (!Cast->IgnoreParenImpCasts()->isEvaluatable(C.getASTContext())) {
+LossOfSign = isLossOfSign(Cast, C);
+LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+ }
} else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
// No loss of sign.
LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
@@ -95,7 +99,12 @@
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
- } else if (isa(Parent)) {
+ } else if (isa(Parent) || isa(Parent)) {
+if (!Cast->IgnoreParenImpCasts()->isEvaluatable(C.getASTContext())) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+}
+ } else {
LossOfSign = isLossOfSign(Cast, C);
LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 added a comment. So moving to core will require explicit casts in some of the tests, especially for things like: `memcpy(a262.s1, input, -1)`. Or this could be moved to another section instead of core. In https://reviews.llvm.org/D45532 there is talk of adding a bugprone section. I think this would be good there. Repository: rC Clang https://reviews.llvm.org/D46081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 added a comment. > Have you tried on any large codebases? This check is not available to the user yet. I can move it to core if you want. Repository: rC Clang https://reviews.llvm.org/D46081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:77
if (Opc == BO_Assign) {
- LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+ if (!B->getRHS()->isIntegerConstantExpr(C.getASTContext())) {
+LossOfSign = isLossOfSign(Cast, C);
george.karpenkov wrote:
> Sorry, I don't quite follow: why we don't want to warn when RHS is an integer
> constant?
Because we want to treat `unsigned i = -1` as an explicit conversion. Ideally,
I would like to just check that the RHS is a literal, but this seems like the
closest way to do that.
Repository:
rC Clang
https://reviews.llvm.org/D46081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 added a comment.
> While I understand extending the analyzer to cover more is a good approach,
> there is -Wconversion which seemingly covers this -- or at least the trivial
> case(?):
Yes, but it won't catch something like this, which is more interesting:
void g(unsigned);
void f(int x) {
if (x < 0) return;
g(x - 1);
}
Repository:
rC Clang
https://reviews.llvm.org/D46081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers
pfultz2 abandoned this revision. pfultz2 added a comment. So I submitted my change to the ConversionChecker, instead. Repository: rC Clang https://reviews.llvm.org/D46066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow
pfultz2 created this revision.
pfultz2 added reviewers: NoQ, xazax.hun, dkrupp, whisperity.
pfultz2 added a project: clang.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.
This expands checking for more expressions. This will check underflow and loss
of precision when using call expressions like:
void foo(unsigned);
int i = -1;
foo(i);
This also includes other expressions as well, so it can catch negative indices
to `std::vector` since it uses unsigned integers for `[]` and `.at()` function.
Repository:
rC Clang
https://reviews.llvm.org/D46081
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -102,6 +102,23 @@
S = U / S; // expected-warning {{Loss of sign}}
}
+void f(unsigned x) {}
+void g(unsigned x) {}
+
+void functioncall1() {
+ long x = -1;
+ int y = 0;
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+}
+
+void functioncall2(int x, int y) {
+ if (x < 0)
+f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+}
+
void dontwarn1(unsigned U, signed S) {
U8 = S; // It might be known that S is always 0x00-0xff.
S8 = U; // It might be known that U is always 0x00-0xff.
@@ -129,12 +146,17 @@
DOSTUFF;
}
-// don't warn for calculations
-// seen some fp. For instance: c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a'
: c2;
-// there is a todo in the checker to handle calculations
void dontwarn5() {
- signed S = -32;
- U8 = S + 10;
+ unsigned char c1 = 'A';
+ c1 = (c1 >= 'A' && c1 <= 'Z') ? c1 - 'A' + 'a' : c1;
+ unsigned char c2 = 0;
+ c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+ unsigned char c3 = 'Z';
+ c3 = (c3 >= 'A' && c3 <= 'Z') ? c3 - 'A' + 'a' : c3;
+ unsigned char c4 = 'a';
+ c4 = (c4 >= 'A' && c4 <= 'Z') ? c4 - 'A' + 'a' : c4;
+ unsigned char c5 = '@';
+ c5 = (c5 >= 'A' && c5 <= 'Z') ? c5 - 'A' + 'a' : c5;
}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -53,9 +53,8 @@
void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
CheckerContext ) const {
- // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
- // calculations also.
- if (!isa(Cast->IgnoreParenImpCasts()))
+ // Don't warn for implicit conversions to bool
+ if (Cast->getType()->isBooleanType())
return;
// Don't warn for loss of sign/precision in macros.
@@ -75,8 +74,10 @@
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
if (Opc == BO_Assign) {
- LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+ if (!B->getRHS()->isIntegerConstantExpr(C.getASTContext())) {
+LossOfSign = isLossOfSign(Cast, C);
+LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+ }
} else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
// No loss of sign.
LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
@@ -95,7 +96,7 @@
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
- } else if (isa(Parent)) {
+ } else {
LossOfSign = isLossOfSign(Cast, C);
LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
}
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -102,6 +102,23 @@
S = U / S; // expected-warning {{Loss of sign}}
}
+void f(unsigned x) {}
+void g(unsigned x) {}
+
+void functioncall1() {
+ long x = -1;
+ int y = 0;
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+}
+
+void functioncall2(int x, int y) {
+ if (x < 0)
+f(x); // expected-warning {{Loss of sign in implicit conversion}}
+ f(y);
+ f(x); // expected-warning {{Loss of sign in implicit conversion}}
+}
+
void dontwarn1(unsigned U, signed S) {
U8 = S; // It might be known that S is always 0x00-0xff.
S8 = U; // It might be known that U is always 0x00-0xff.
@@ -129,12 +146,17 @@
DOSTUFF;
}
-// don't warn for calculations
-// seen some fp. For instance: c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
-// there is a todo in the checker to handle calculations
void dontwarn5() {
- signed S = -32;
- U8 = S + 10;
+ unsigned char c1 = 'A';
+ c1 = (c1 >= 'A' && c1 <= 'Z') ? c1 - 'A' + 'a' : c1;
+ unsigned char c2 = 0;
+ c2 = (c2 >= 'A' && c2 <=
[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers
pfultz2 added a comment. > Isn't this case already covered by conversion checker? I was unaware of this. This looks like it only works for binary operators. So `f(-1)` won't get caught. Repository: rC Clang https://reviews.llvm.org/D46066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46066: [analyzer] Add checker for underflowing unsigned integers
pfultz2 created this revision.
pfultz2 added reviewers: NoQ, xazax.hun, dkrupp, whisperity, george.karpenkov.
pfultz2 added a project: clang.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet, mgorny.
This will check for when assigning a negative value to an unsigned integer,
when it happens implicitly.
Repository:
rC Clang
https://reviews.llvm.org/D46066
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/UnderflowChecker.cpp
test/Analysis/underflow.cpp
Index: test/Analysis/underflow.cpp
===
--- /dev/null
+++ test/Analysis/underflow.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.Underflow -verify %s
+
+void foo(unsigned int i);
+
+void f1() {
+ unsigned int i = -1; // expected-warning {{Underflow unsigned integer}}
+ unsigned int j = 0;
+}
+
+void f2() {
+ foo(-1); // expected-warning {{Underflow unsigned integer}}
+ foo(0);
+}
+
+void f3() {
+ long x = -1;
+ int y = 0;
+ foo(x); // expected-warning {{Underflow unsigned integer}}
+ foo(y);
+}
Index: lib/StaticAnalyzer/Checkers/UnderflowChecker.cpp
===
--- /dev/null
+++ lib/StaticAnalyzer/Checkers/UnderflowChecker.cpp
@@ -0,0 +1,93 @@
+//== UnderflowCheck.cpp - Division by zero checker --*- C++ -*--==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This defines UnderflowCheck, a builtin check in ExprEngine that performs
+// checks for assigning negative values to unsigned integers
+//
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class UnderflowCheck : public Checker {
+ mutable std::unique_ptr BT;
+ void reportBug(const char *Msg, ProgramStateRef StateZero,
+ CheckerContext ) const;
+
+public:
+ void checkPreStmt(const ImplicitCastExpr *CE, CheckerContext ) const;
+};
+} // end anonymous namespace
+
+void UnderflowCheck::reportBug(const char *Msg, ProgramStateRef StateZero,
+ CheckerContext ) const {
+ if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
+if (!BT)
+ BT.reset(new BuiltinBug(this, "Underflow unsigned integer"));
+C.emitReport(llvm::make_unique(*BT, BT->getDescription(), N));
+ }
+}
+
+void UnderflowCheck::checkPreStmt(const ImplicitCastExpr *CE,
+ CheckerContext ) const {
+
+ if (!CE->getType()->isUnsignedIntegerType() || CE->getType()->isBooleanType())
+return;
+
+ const Expr *E = CE->getSubExpr();
+ if (!E)
+return;
+ QualType valTy = E->getType();
+ if (valTy->isUnsignedIntegerType())
+return;
+ Optional DV = C.getSVal(E).getAs();
+
+ if (!DV)
+return;
+
+ // Check for negative
+ ProgramStateRef state = C.getState();
+ SValBuilder = C.getSValBuilder();
+ ConstraintManager = C.getConstraintManager();
+
+ // First, ensure that the value is >= 0.
+ DefinedSVal zeroVal = svalBuilder.makeIntVal(0, valTy);
+ SVal greaterThanOrEqualToZeroVal = svalBuilder.evalBinOp(
+ state, BO_GE, *DV, zeroVal, svalBuilder.getConditionType());
+
+ Optional greaterThanEqualToZero =
+ greaterThanOrEqualToZeroVal.getAs();
+
+ if (!greaterThanEqualToZero) {
+// The SValBuilder cannot construct a valid SVal for this condition.
+// This means we cannot properly reason about it.
+return;
+ }
+
+ ProgramStateRef stateLT, stateGE;
+ std::tie(stateGE, stateLT) = CM.assumeDual(state, *greaterThanEqualToZero);
+
+ // Is it possible for the value to be less than zero?
+ if (stateLT && !stateGE) {
+reportBug("Underflow unsigned integer", stateLT, C);
+// In either case, we are done.
+return;
+ }
+}
+
+void ento::registerUnderflowChecker(CheckerManager ) {
+ mgr.registerChecker();
+}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -91,6 +91,7 @@
UndefResultChecker.cpp
UndefinedArraySubscriptChecker.cpp
UndefinedAssignmentChecker.cpp
+ UnderflowChecker.cpp
UnixAPIChecker.cpp
UnreachableCodeChecker.cpp
VforkChecker.cpp
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.
pfultz2 added a comment. This seems like it would be nice to have in `bugprone-*` as well. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 updated this revision to Diff 140790.
pfultz2 added a comment.
I have rebased.
https://reviews.llvm.org/D44231
Files:
clang-tidy/bugprone/SizeofExpressionCheck.cpp
clang-tidy/bugprone/SizeofExpressionCheck.h
docs/clang-tidy/checks/bugprone-sizeof-expression.rst
test/clang-tidy/bugprone-sizeof-expression.cpp
Index: test/clang-tidy/bugprone-sizeof-expression.cpp
===
--- test/clang-tidy/bugprone-sizeof-expression.cpp
+++ test/clang-tidy/bugprone-sizeof-expression.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -config="{CheckOptions: [{key: bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression, value: 1}]}" --
class C {
int size() { return sizeof(this); }
@@ -14,14 +14,82 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+enum class EC { VALUE = 0 };
+
+bool AsBool() { return false; }
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+EC AsEnumClass() { return EC::VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
+int ReturnOverload(int) { return {}; }
+S ReturnOverload(S) { return {}; }
+
+template
+T ReturnTemplate(T) { return {}; }
+
+template
+bool TestTrait1() {
+ return sizeof(ReturnOverload(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait2() {
+ return sizeof(ReturnTemplate(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait3() {
+ return sizeof(ReturnOverload(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+template
+bool TestTrait4() {
+ return sizeof(ReturnTemplate(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+bool TestTemplates() {
+ bool b = true;
+ b &= TestTrait1();
+ b &= TestTrait1();
+ b &= TestTrait2();
+ b &= TestTrait2();
+ b &= TestTrait3();
+ b &= TestTrait3();
+ b &= TestTrait4();
+ b &= TestTrait4();
+ return b;
+}
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(LEN + 1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsBool());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsEnumClass());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +239,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: docs/clang-tidy/checks/bugprone-sizeof-expression.rst
===
--- docs/clang-tidy/checks/bugprone-sizeof-expression.rst
+++ docs/clang-tidy/checks/bugprone-sizeof-expression.rst
@@ -22,6 +22,36 @@
char buf[BUFLEN];
memset(buf, 0, sizeof(BUFLEN)); // sizeof(42) ==> sizeof(int)
+Suspicious usage of 'sizeof(expr)'
+--
+
+In cases, where there is an enum or integer to represent a type, a common
+mistake is to query the ``sizeof`` on the integer or enum that represents the
+type that should be used by ``sizeof``. This results in the size of the integer
+and not of the type the integer represents:
+
+.. code-block:: c++
+
+ enum data_type {
+FLOAT_TYPE,
+DOUBLE_TYPE
+ };
+
+ struct data {
+data_type type;
+void* buffer;
+data_type get_type() {
+ return type;
+}
+ };
+
+ void f(data d, int numElements) {
+// should be sizeof(float) or sizeof(double), depending on d.get_type()
+
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. Is someone able to merge in my changes? https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > Do you need someone to submit this for you? Yes https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. So, I've added tests for class enums and bols. https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 updated this revision to Diff 139649.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index: test/clang-tidy/misc-sizeof-expression.cpp
===
--- test/clang-tidy/misc-sizeof-expression.cpp
+++ test/clang-tidy/misc-sizeof-expression.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-sizeof-expression %t
+// RUN: %check_clang_tidy %s misc-sizeof-expression %t -- -config="{CheckOptions: [{key: misc-sizeof-expression.WarnOnSizeOfIntegerExpression, value: 1}]}" --
class C {
int size() { return sizeof(this); }
@@ -14,14 +14,82 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+enum class EC { VALUE = 0 };
+
+bool AsBool() { return false; }
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+EC AsEnumClass() { return EC::VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
+int ReturnOverload(int) { return {}; }
+S ReturnOverload(S) { return {}; }
+
+template
+T ReturnTemplate(T) { return {}; }
+
+template
+bool TestTrait1() {
+ return sizeof(ReturnOverload(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait2() {
+ return sizeof(ReturnTemplate(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait3() {
+ return sizeof(ReturnOverload(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+template
+bool TestTrait4() {
+ return sizeof(ReturnTemplate(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+}
+
+bool TestTemplates() {
+ bool b = true;
+ b &= TestTrait1();
+ b &= TestTrait1();
+ b &= TestTrait2();
+ b &= TestTrait2();
+ b &= TestTrait3();
+ b &= TestTrait3();
+ b &= TestTrait4();
+ b &= TestTrait4();
+ return b;
+}
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(LEN + 1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsBool());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(AsEnumClass());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +239,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: docs/clang-tidy/checks/misc-sizeof-expression.rst
===
--- docs/clang-tidy/checks/misc-sizeof-expression.rst
+++ docs/clang-tidy/checks/misc-sizeof-expression.rst
@@ -22,6 +22,36 @@
char buf[BUFLEN];
memset(buf, 0, sizeof(BUFLEN)); // sizeof(42) ==> sizeof(int)
+Suspicious usage of 'sizeof(expr)'
+--
+
+In cases, where there is an enum or integer to represent a type, a common
+mistake is to query the ``sizeof`` on the integer or enum that represents the
+type that should be used by ``sizeof``. This results in the size of the integer
+and not of the type the integer represents:
+
+.. code-block:: c++
+
+ enum data_type {
+FLOAT_TYPE,
+DOUBLE_TYPE
+ };
+
+ struct data {
+data_type type;
+void* buffer;
+data_type get_type() {
+ return type;
+}
+ };
+
+ void f(data d, int numElements) {
+// should be sizeof(float) or sizeof(double), depending on d.get_type()
+int numBytes = numElements * sizeof(d.get_type());
+...
+ }
+
+
Suspicious usage of
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 marked an inline comment as done. pfultz2 added a comment. I have reworded the documentation. Hopefully it is clearer. https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > As this patch can catch some mistakes, I'm fine with checking it in. I agree > that it is reasonable to write normal code like sizeof(func_call()) (not > false positive), maybe set the option to false by default? I have disabled it by default. We can decide later if we want it to enabled by default. https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 updated this revision to Diff 138791.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index: test/clang-tidy/misc-sizeof-expression.cpp
===
--- test/clang-tidy/misc-sizeof-expression.cpp
+++ test/clang-tidy/misc-sizeof-expression.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-sizeof-expression %t
+// RUN: %check_clang_tidy %s misc-sizeof-expression %t -- -config="{CheckOptions: [{key: misc-sizeof-expression.WarnOnSizeOfIntegerExpression, value: 1}]}" --
class C {
int size() { return sizeof(this); }
@@ -14,14 +14,75 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
+int ReturnOverload(int) { return {}; }
+S ReturnOverload(S) { return {}; }
+
+template
+T ReturnTemplate(T) { return {}; }
+
+template
+bool TestTrait1() {
+ return sizeof(ReturnOverload(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait2() {
+ return sizeof(ReturnTemplate(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait3() {
+ return sizeof(ReturnOverload(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+}
+
+template
+bool TestTrait4() {
+ return sizeof(ReturnTemplate(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+}
+
+bool TestTemplates() {
+ bool b = true;
+ b &= TestTrait1();
+ b &= TestTrait1();
+ b &= TestTrait2();
+ b &= TestTrait2();
+ b &= TestTrait3();
+ b &= TestTrait3();
+ b &= TestTrait4();
+ b &= TestTrait4();
+ return b;
+}
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(LEN + 1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +232,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: docs/clang-tidy/checks/misc-sizeof-expression.rst
===
--- docs/clang-tidy/checks/misc-sizeof-expression.rst
+++ docs/clang-tidy/checks/misc-sizeof-expression.rst
@@ -22,6 +22,36 @@
char buf[BUFLEN];
memset(buf, 0, sizeof(BUFLEN)); // sizeof(42) ==> sizeof(int)
+Suspicious usage of 'sizeof(expr)'
+--
+
+In cases, where there is an enum or integer to represent a type, a common
+mistake is to query the ``sizeof`` on the integer or enum that represents the
+type that should be used by ``sizeof``. This results in the size of the integer
+and not of the type the integer represents:
+
+.. code-block:: c++
+
+ enum data_type {
+FLOAT_TYPE,
+DOUBLE_TYPE
+ };
+
+ struct data {
+data_type type;
+void* buffer;
+data_type get_type() {
+ return type;
+}
+ };
+
+ void f(data d, int numElements) {
+// should be sizeof(float) or sizeof(double), depending on d.get_type()
+int numBytes = numElements * sizeof(d.get_type());
+...
+ }
+
+
Suspicious usage of 'sizeof(this)'
--
@@ -142,6 +172,11 @@
When non-zero, the check will warn on an expression like
``sizeof(CONSTANT)``. Default is `1`.
+.. option:: WarnOnSizeOfIntegerExpression
+
+ When non-zero, the check will warn on an expression like ``sizeof(expr)``
+ where the expression results in an integer. Default is `0`.
+
.. option:: WarnOnSizeOfThis
When non-zero, the
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. So I've updated the documentation on this. Are there any other updates needed for this to get it merged in? https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 updated this revision to Diff 137822.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index: test/clang-tidy/misc-sizeof-expression.cpp
===
--- test/clang-tidy/misc-sizeof-expression.cpp
+++ test/clang-tidy/misc-sizeof-expression.cpp
@@ -14,14 +14,75 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
+int ReturnOverload(int) { return {}; }
+S ReturnOverload(S) { return {}; }
+
+template
+T ReturnTemplate(T) { return {}; }
+
+template
+bool TestTrait1() {
+ return sizeof(ReturnOverload(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait2() {
+ return sizeof(ReturnTemplate(T{})) == sizeof(A);
+}
+
+template
+bool TestTrait3() {
+ return sizeof(ReturnOverload(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+}
+
+template
+bool TestTrait4() {
+ return sizeof(ReturnTemplate(0)) == sizeof(T{});
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+}
+
+bool TestTemplates() {
+ bool b = true;
+ b &= TestTrait1();
+ b &= TestTrait1();
+ b &= TestTrait2();
+ b &= TestTrait2();
+ b &= TestTrait3();
+ b &= TestTrait3();
+ b &= TestTrait4();
+ b &= TestTrait4();
+ return b;
+}
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(LEN + 1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof() on an expression that results in an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +232,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: docs/clang-tidy/checks/misc-sizeof-expression.rst
===
--- docs/clang-tidy/checks/misc-sizeof-expression.rst
+++ docs/clang-tidy/checks/misc-sizeof-expression.rst
@@ -22,6 +22,26 @@
char buf[BUFLEN];
memset(buf, 0, sizeof(BUFLEN)); // sizeof(42) ==> sizeof(int)
+Suspicious usage of 'sizeof(expr)'
+--
+
+A common mistake is to query the ``sizeof`` on an integer or enum that
+represents the type, which will give the size of the integer and not of the
+type the integer represents:
+
+.. code-block:: c++
+
+ enum data_type {
+FLOAT_TYPE,
+DOUBLE_TYPE
+ };
+
+ data_type get_type() {
+return FLOAT_TYPE;
+ }
+
+ int numBytes = numElements * sizeof(get_type()); // should be sizeof(float)
+
Suspicious usage of 'sizeof(this)'
--
@@ -142,6 +162,11 @@
When non-zero, the check will warn on an expression like
``sizeof(CONSTANT)``. Default is `1`.
+.. option:: WarnOnSizeOfIntegerExpression
+
+ When non-zero, the check will warn on an expression like ``sizeof(expr)``
+ where the expression results in an integer. Default is `1`.
+
.. option:: WarnOnSizeOfThis
When non-zero, the check will warn on an expression like ``sizeof(this)``.
Index: clang-tidy/misc/SizeofExpressionCheck.h
===
--- clang-tidy/misc/SizeofExpressionCheck.h
+++ clang-tidy/misc/SizeofExpressionCheck.h
@@ -29,6 +29,7 @@
private:
const bool WarnOnSizeOfConstant;
+ const bool WarnOnSizeOfIntegerExpression;
const bool WarnOnSizeOfThis;
const bool WarnOnSizeOfCompareToConstant;
};
Index: clang-tidy/misc/SizeofExpressionCheck.cpp
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > I don't have a script for it. I've used "bear" with at least some of those > projects because they use makefiles rather than cmake > (https://github.com/rizsotto/Bear). I'm not tied to those projects > specifically, either, so if you have a different corpus you'd prefer to use, > that's fine. The important bit is testing it across a considerable amount of > C code and C++ code to see whether this diagnostic is too chatty or not. So I did a grep over these codebases(with `grep -E '\bsizeof\([^()"*]+\([^()]*\)'`). Most of them are macros which access elements(ie no function call) or are used in type traits. The only false positive I saw was here: https://github.com/rethinkdb/rethinkdb/blob/v2.3.x/external/re2_20140111/re2/prog.cc#L317 So I dont think it will be too chatty. > That won't catch many (most?) of the issues demonstrated by PVS-Studio; the > rule their check follows are to warn on side-effecting operations (which > Clang already does with -Wunevaluated-expression) and arithmetic expressions > in sizeof. It finds function calls as well. I tried on MIOpen and it caught the errors like I mentioned earlier here: https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > Again, that only works for C++ and not C. Typedef has always worked in C. > Did it report any true positives that would need correcting? Not for LLVM, but it has in other projects like I mentioned. > Can you check some other large repos (both C++ and C), such as: Qt, cocos2d, > rethinkdb, redis, and php-src? None of those projects use cmake, so it doesn't look easy to run clang-tidy on them. Do you have a script that will run clang-tidy on these repos? Actually, PVS-Studio has a more general check for `sizeof(expr)`: https://www.viva64.com/en/examples/v568/ It shows an example of FCEUX doing `sizeof(buf - 1)`, Asterisk doing `sizeof(data[0] * 2)` and ReactOS doing `sizeof(UnknownError [0] - 20)`. I think it might be a good idea to expand this from just a function call to any integer expression. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > If the return type of foo() is changed, then the use for s1 will be > automatically updated But usually you write it as: using foo_return = uint16_t; foo_return foo(); ... size_t s1 = sizeof(foo()); size_t s2 = sizeof(foo_return); So you just update the `foo_return` typedef, and it will update it everywhere that type is used. > I think we need some data measurements over several large (>500k LoC) C and > C++ code bases to see how frequently this check yields false positives. So I ran it on llvm. It mainly complained about areas where `sizeof` is used for building traits. I should add an extra check that the function is not overloaded. It did complain once about `sizeof` in a detection idiom(in BitmaskEnum.h) and once with `sizeof(getVersion())`, like you mentioned. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment. > Can you point to some real world code that would benefit from this check? Yes, here: https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184 It is erroneously calling `sizeof(yDesc.GetType())`. The `GetType` function returns an enum that represents the float type. In this case it wants to compute the size of the float type not the size of the enum that represents the float type. There's actually many instances of this error in the codebase. > I don't see this as being a common issue with code and I am concerned about > false positives. I have seen it as a common issue. It only runs for a function that returns an integer type. Another possibility is to not warn for function calls that are dependent on a template parameter. Of course, there is still the option to disable it. > For instance, it's reasonable to write sizeof(func_call()) in a context where > you don't want to repeat the type name in multiple places. Generally, a typedef is used to avoid repeating the type. > I've seen this construct used in LLVM's code base (see Prologue::getLength() > in DWARFDebugLine.h for one such instance). This is the first time I have seen this used legitimately outside of template metaprogramming. Alternatively, you could also write `sizeof(decltype(expr))`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44241: [clang-tidy] Add a check for constructing non-trivial types without assigning to a variable
pfultz2 added a comment.
> How does this differ from misc-unused-raii?
Actually, I was unaware of this check. This seems like a better place to start.
Whats missing from the misc-unused-raii is that it doesn't catch it when
constructing with curly braces(ie `NonTrivial{}`). Let me work on updating that
check instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: [clang-tidy] Check for sizeof that call functions
pfultz2 added a comment.
> Can you elaborate a bit more about this?
This catches problems when calling `sizeof(f())` when `f` returns an integer(or
enum). This is because, most likely, the integer represents the type to be
chosen, however, `sizeof` is being computed for an integer and not the type the
integer represents. That is, the user has an enum for the `data_type`:
enum data_type {
float_type,
double_type
};
At some point the user may call a function to get the data type(perhaps
`x.GetType()`) and pass it on to `sizeof`, like `sizeof(x.GetType())`, which is
incorrect.
> I think we also need to update the check document (adding proper section of
> this new behavior, and the new option).
I will update the doc.
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:220
+ Result.Nodes.getNodeAs("sizeof-integer-call")) {
+diag(E->getLocStart(), "suspicious usage of 'sizeof(expr)' to an integer");
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
alexfh wrote:
> I'm not sure I understand the message "suspicious usage of ... to an
> integer". Specifically, what does the "to an integer" part mean?
That could probably be worded better. It means the `expr` is an integer type.
Maybe I should say `suspicious usage of 'sizeof() on an expression that results
in an integer`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44241: Add a check for constructing non-trivial types without assigning to a variable
pfultz2 created this revision.
pfultz2 added reviewers: clang-tools-extra, aaron.ballman.
pfultz2 added a project: clang-tools-extra.
Herald added a subscriber: mgorny.
Diagnoses when a non-trivial type is not assigned to a variable. This is useful
to check for problems like unnamed lock guards.
For example, if you had code written like this:
int g_i = 0;
std::mutex g_i_mutex; // protects g_i
void safe_increment()
{
std::lock_guard{g_i_mutex};
// The lock is locked and then unlocked before reaching here
++g_i;
}
This will warn that a variable should be created instead:
int g_i = 0;
std::mutex g_i_mutex; // protects g_i
void safe_increment()
{
std::lock_guard lock{g_i_mutex};
// The lock is locked and then will be unlocked when exiting scope
++g_i;
}
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44241
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/OwnerlessResourceCheck.cpp
clang-tidy/misc/OwnerlessResourceCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-ownerless-resource.rst
test/clang-tidy/misc-ownerless-resource.cpp
Index: test/clang-tidy/misc-ownerless-resource.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-ownerless-resource.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s misc-ownerless-resource %t
+
+struct Trivial {};
+
+struct NonTrivial {
+ int *mem;
+ NonTrivial()
+ : mem(new int()) {}
+ NonTrivial(int)
+ : mem(new int()) {}
+ ~NonTrivial() {
+delete mem;
+ }
+};
+
+NonTrivial f() { return {}; }
+void g(NonTrivial) {}
+
+void fail() {
+ NonTrivial{};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Resource has no owner; Did you mean to declare it with a variable name? [misc-ownerless-resource]
+ NonTrivial{1};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Resource has no owner; Did you mean to declare it with a variable name? [misc-ownerless-resource]
+}
+
+void valid() {
+ NonTrivial a{};
+ auto b = NonTrivial{};
+
+ g(f());
+ f();
+
+ auto c = f();
+ Trivial{};
+ Trivial x{};
+}
Index: docs/clang-tidy/checks/misc-ownerless-resource.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-ownerless-resource.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - misc-ownerless-resource
+
+misc-ownerless-resource
+===
+
+Diagnoses when a non-trivial type is not assigned to a variable. This is useful to check for problems like unamed lock guards.
+
+For example, if you had code written like this:
+
+.. code-block:: c++
+
+ int g_i = 0;
+ std::mutex g_i_mutex; // protects g_i
+
+ void safe_increment()
+ {
+std::lock_guard{g_i_mutex};
+// The lock is locked and then unlocked before reaching here
+++g_i;
+ }
+
+This will warn that a variable should be created instead:
+
+.. code-block:: c++
+
+ int g_i = 0;
+ std::mutex g_i_mutex; // protects g_i
+
+ void safe_increment()
+ {
+std::lock_guard lock{g_i_mutex};
+// The lock is locked and then will be unlocked when exiting scope
+++g_i;
+ }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -143,6 +143,7 @@
misc-misplaced-const
misc-new-delete-overloads
misc-non-copyable-objects
+ misc-ownerless-resource
misc-redundant-expression
misc-sizeof-container
misc-sizeof-expression
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
Improvements to clang-tidy
--
+- New `misc-ownerless-resource
+ `_ check
+
+ Diagnoses when a non-trivial type is not assigned to a variable. This is
+ useful to check for problems like unamed lock guards.
+
- New `bugprone-throw-keyword-missing
`_ check
Index: clang-tidy/misc/OwnerlessResourceCheck.h
===
--- /dev/null
+++ clang-tidy/misc/OwnerlessResourceCheck.h
@@ -0,0 +1,35 @@
+//===--- OwnerlessResourceCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OWNERLESSRESOURCECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OWNERLESSRESOURCECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace
[PATCH] D44231: Check for sizeof that call functions
pfultz2 updated this revision to Diff 137499.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
test/clang-tidy/misc-sizeof-expression.cpp
Index: test/clang-tidy/misc-sizeof-expression.cpp
===
--- test/clang-tidy/misc-sizeof-expression.cpp
+++ test/clang-tidy/misc-sizeof-expression.cpp
@@ -14,14 +14,34 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(LEN + 1);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +191,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: clang-tidy/misc/SizeofExpressionCheck.h
===
--- clang-tidy/misc/SizeofExpressionCheck.h
+++ clang-tidy/misc/SizeofExpressionCheck.h
@@ -29,6 +29,7 @@
private:
const bool WarnOnSizeOfConstant;
+ const bool WarnOnSizeOfCall;
const bool WarnOnSizeOfThis;
const bool WarnOnSizeOfCompareToConstant;
};
Index: clang-tidy/misc/SizeofExpressionCheck.cpp
===
--- clang-tidy/misc/SizeofExpressionCheck.cpp
+++ clang-tidy/misc/SizeofExpressionCheck.cpp
@@ -62,12 +62,14 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", 1) != 0),
+ WarnOnSizeOfCall(Options.get("WarnOnSizeOfCall", 1) != 0),
WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", 1) != 0),
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", 1) != 0) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
+ Options.store(Opts, "WarnOnSizeOfCall", WarnOnSizeOfCall);
Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
Options.store(Opts, "WarnOnSizeOfCompareToConstant",
WarnOnSizeOfCompareToConstant);
@@ -78,6 +80,8 @@
const auto ConstantExpr = expr(ignoringParenImpCasts(
anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
binaryOperator(hasLHS(IntegerExpr), hasRHS(IntegerExpr);
+ const auto IntegerCallExpr =
+ expr(ignoringParenImpCasts(callExpr(hasType(isInteger();
const auto SizeOfExpr =
expr(anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr();
const auto SizeOfZero = expr(
@@ -94,6 +98,14 @@
this);
}
+ // Detect sizeof(f())
+ if (WarnOnSizeOfCall) {
+Finder->addMatcher(
+expr(sizeOfExpr(ignoringParenImpCasts(has(IntegerCallExpr
+.bind("sizeof-integer-call"),
+this);
+ }
+
// Detect expression like: sizeof(this);
if (WarnOnSizeOfThis) {
Finder->addMatcher(
@@ -203,6 +215,9 @@
if (const auto *E = Result.Nodes.getNodeAs("sizeof-constant")) {
diag(E->getLocStart(),
"suspicious usage of 'sizeof(K)'; did you mean 'K'?");
+ } else if (const auto *E =
+ Result.Nodes.getNodeAs("sizeof-integer-call")) {
+diag(E->getLocStart(), "suspicious usage of 'sizeof(expr)' to an integer");
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
diag(E->getLocStart(),
"suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: Check for sizeof that call functions
pfultz2 updated this revision to Diff 137497.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
test/clang-tidy/misc-sizeof-expression.cpp
Index: test/clang-tidy/misc-sizeof-expression.cpp
===
--- test/clang-tidy/misc-sizeof-expression.cpp
+++ test/clang-tidy/misc-sizeof-expression.cpp
@@ -14,6 +14,18 @@
#pragma pack(1)
struct S { char a, b, c; };
+enum E { E_VALUE = 0 };
+
+int AsInt() { return 0; }
+E AsEnum() { return E_VALUE; }
+S AsStruct() { return {}; }
+
+struct M {
+ int AsInt() { return 0; }
+ E AsEnum() { return E_VALUE; }
+ S AsStruct() { return {}; }
+};
+
int Test1(const char* ptr) {
int sum = 0;
sum += sizeof(LEN);
@@ -22,6 +34,14 @@
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
sum += sizeof(sum, LEN);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+ sum += sizeof(AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(M{}.AsInt());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
+ sum += sizeof(M{}.AsEnum());
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(expr)' to an integer
sum += sizeof(sizeof(X));
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
sum += sizeof(LEN + sizeof(X));
@@ -171,6 +191,8 @@
if (sizeof(A) < 10)
sum += sizeof(A);
sum += sizeof(int);
+ sum += sizeof(AsStruct());
+ sum += sizeof(M{}.AsStruct());
sum += sizeof(A[sizeof(A) / sizeof(int)]);
sum += sizeof([sizeof(A) / sizeof(int)]);
sum += sizeof(sizeof(0)); // Special case: sizeof size_t.
Index: clang-tidy/misc/SizeofExpressionCheck.h
===
--- clang-tidy/misc/SizeofExpressionCheck.h
+++ clang-tidy/misc/SizeofExpressionCheck.h
@@ -29,6 +29,7 @@
private:
const bool WarnOnSizeOfConstant;
+ const bool WarnOnSizeOfCall;
const bool WarnOnSizeOfThis;
const bool WarnOnSizeOfCompareToConstant;
};
Index: clang-tidy/misc/SizeofExpressionCheck.cpp
===
--- clang-tidy/misc/SizeofExpressionCheck.cpp
+++ clang-tidy/misc/SizeofExpressionCheck.cpp
@@ -62,12 +62,14 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", 1) != 0),
+ WarnOnSizeOfCall(Options.get("WarnOnSizeOfCall", 1) != 0),
WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", 1) != 0),
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", 1) != 0) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
+ Options.store(Opts, "WarnOnSizeOfCall", WarnOnSizeOfCall);
Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
Options.store(Opts, "WarnOnSizeOfCompareToConstant",
WarnOnSizeOfCompareToConstant);
@@ -78,6 +80,8 @@
const auto ConstantExpr = expr(ignoringParenImpCasts(
anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
binaryOperator(hasLHS(IntegerExpr), hasRHS(IntegerExpr);
+ const auto IntegerCallExpr =
+ expr(ignoringParenImpCasts(callExpr(hasType(isInteger();
const auto SizeOfExpr =
expr(anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr();
const auto SizeOfZero = expr(
@@ -94,6 +98,14 @@
this);
}
+ // Detect sizeof(f())
+ if (WarnOnSizeOfCall) {
+Finder->addMatcher(
+expr(sizeOfExpr(ignoringParenImpCasts(has(IntegerCallExpr
+.bind("sizeof-integer-call"),
+this);
+ }
+
// Detect expression like: sizeof(this);
if (WarnOnSizeOfThis) {
Finder->addMatcher(
@@ -203,6 +215,9 @@
if (const auto *E = Result.Nodes.getNodeAs("sizeof-constant")) {
diag(E->getLocStart(),
"suspicious usage of 'sizeof(K)'; did you mean 'K'?");
+ } else if (const auto *E =
+ Result.Nodes.getNodeAs("sizeof-integer-call")) {
+diag(E->getLocStart(), "suspicious usage of 'sizeof(expr)' to an integer");
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
diag(E->getLocStart(),
"suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: Check for sizeof that call functions
pfultz2 updated this revision to Diff 137496.
https://reviews.llvm.org/D44231
Files:
clangd/CodeComplete.cpp
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -595,6 +595,18 @@
EXPECT_TRUE(Results.items.empty());
}
+TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
+ auto Results = completions(R"cpp(
+namespace clang { }
+void f() {
+ clan^
+}
+ )cpp");
+
+ EXPECT_THAT(Results.items, Contains(Labeled("clang")));
+ EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
+}
+
SignatureHelp signatures(StringRef Text) {
MockFSProvider FS;
MockCompilationDatabase CDB;
Index: test/clangd/protocol.test
===
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -38,7 +38,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -67,7 +67,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -96,7 +96,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -470,6 +470,8 @@
// (s.^ completes ~string, but s.~st^ is an error).
if (dyn_cast_or_null(Result.Declaration))
continue;
+ // We choose to never append '::' to completion results in clangd.
+ Result.StartsNestedNameSpecifier = false;
Results.push_back(Result);
}
ResultsCallback();
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -595,6 +595,18 @@
EXPECT_TRUE(Results.items.empty());
}
+TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
+ auto Results = completions(R"cpp(
+namespace clang { }
+void f() {
+ clan^
+}
+ )cpp");
+
+ EXPECT_THAT(Results.items, Contains(Labeled("clang")));
+ EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
+}
+
SignatureHelp signatures(StringRef Text) {
MockFSProvider FS;
MockCompilationDatabase CDB;
Index: test/clangd/protocol.test
===
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -38,7 +38,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -67,7 +67,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -96,7 +96,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -470,6 +470,8 @@
// (s.^ completes ~string, but s.~st^ is an error).
if (dyn_cast_or_null(Result.Declaration))
continue;
+ // We choose to never append '::' to completion results in clangd.
+ Result.StartsNestedNameSpecifier = false;
Results.push_back(Result);
}
ResultsCallback();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44231: Check for sizeof that call functions
pfultz2 created this revision.
pfultz2 added reviewers: clang-tools-extra, hokein, alexfh, aaron.ballman,
ilya-biryukov.
Herald added a subscriber: jkorous-apple.
A common mistake that I have found in our codebase is calling a function to get
an integer or enum that represents the type such as:
int numBytes = numElements * sizeof(x.GetType());
So this extends the `sizeof` check to check for these cases. There is also a
`WarnOnSizeOfCall` option so it can be disabled.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44231
Files:
clangd/CodeComplete.cpp
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -595,6 +595,18 @@
EXPECT_TRUE(Results.items.empty());
}
+TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
+ auto Results = completions(R"cpp(
+namespace clang { }
+void f() {
+ clan^
+}
+ )cpp");
+
+ EXPECT_THAT(Results.items, Contains(Labeled("clang")));
+ EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
+}
+
SignatureHelp signatures(StringRef Text) {
MockFSProvider FS;
MockCompilationDatabase CDB;
Index: test/clangd/protocol.test
===
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -38,7 +38,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -67,7 +67,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -96,7 +96,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -470,6 +470,8 @@
// (s.^ completes ~string, but s.~st^ is an error).
if (dyn_cast_or_null(Result.Declaration))
continue;
+ // We choose to never append '::' to completion results in clangd.
+ Result.StartsNestedNameSpecifier = false;
Results.push_back(Result);
}
ResultsCallback();
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -595,6 +595,18 @@
EXPECT_TRUE(Results.items.empty());
}
+TEST(CodeCompleteTest, NoColonColonAtTheEnd) {
+ auto Results = completions(R"cpp(
+namespace clang { }
+void f() {
+ clan^
+}
+ )cpp");
+
+ EXPECT_THAT(Results.items, Contains(Labeled("clang")));
+ EXPECT_THAT(Results.items, Not(Contains(Labeled("clang::";
+}
+
SignatureHelp signatures(StringRef Text) {
MockFSProvider FS;
MockCompilationDatabase CDB;
Index: test/clangd/protocol.test
===
--- test/clangd/protocol.test
+++ test/clangd/protocol.test
@@ -38,7 +38,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -67,7 +67,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
@@ -96,7 +96,7 @@
# CHECK-NEXT:"insertText": "fake",
# CHECK-NEXT:"insertTextFormat": 1,
# CHECK-NEXT:"kind": 7,
-# CHECK-NEXT:"label": "fake::",
+# CHECK-NEXT:"label": "fake",
# CHECK-NEXT:"sortText": "{{.*}}"
# CHECK:]
# CHECK-NEXT: }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -470,6 +470,8 @@
// (s.^ completes ~string, but s.~st^ is an error).
if (dyn_cast_or_null(Result.Declaration))
continue;
+ // We choose to never append '::' to completion results in clangd.
+ Result.StartsNestedNameSpecifier =
