[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta added a comment.

This LGTM, but can't officially accept the patch so please wait for another 
reviewer to accept it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

@Eugene.Zelenko I cannot review/accept my own patch :(.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D144135#4524902 , @PiotrZSL wrote:

> @Eugene.Zelenko I cannot review/accept my own patch :(.

I'm sorry for noise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 543196.
PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

Rebase + Update diagnostic message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,105 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreList, value: '::IgnoredEnum;IgnoredSecondEnum'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' uses a larger base type ('std::int16_t' (aka 'short'), size: 2 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' uses a larger base type ('std::uint16_t' (aka 'unsigned short'), size: 2 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
+
+namespace internal
+{
+
+enum class IgnoredSecondEnum
+{
+unused1 = 1,
+unused2 = 2
+};
+
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+class based on the range of its enumerators. Analyzes the value

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Shivam Gupta via Phabricator via cfe-commits]

xgupta accepted this revision.
xgupta added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3397dae69e09: [clang-tidy] Add performance-enum-size check 
(authored by PiotrZSL).

Changed prior to commit:
  https://reviews.llvm.org/D144135?vs=543196&id=544042#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,105 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreList, value: '::IgnoredEnum;IgnoredSecondEnum'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' uses a larger base type ('std::int16_t' (aka 'short'), size: 2 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' uses a larger base type ('std::uint16_t' (aka 'unsigned short'), size: 2 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::int8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
+
+namespace internal
+{
+
+enum class IgnoredSecondEnum
+{
+unused1 = 1,
+unused2 = 2
+};
+
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Daniil Dudkin via Phabricator via cfe-commits]

unterumarmung added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:127-128
+
+  diag(MatchedDecl->getLocation(), "enum %0 derive from %1 of size %2 bytes, "
+   "derive from '%3' to reduce it size to %4")
+  << MatchedDecl << MatchedDecl->getIntegerType() << Size << NewType.first

I find the enum's type derivation message to be a bit unintuitive. It would 
slightly improve the user experience if the error message provided clearer 
information, like stating "enum %0 has a base type of %1..." or "the base type 
of enum %0 appears excessive for its value set...". However, please remember 
that these are merely personal thoughts, and as a non-contributor, my 
suggestions are not obligatory.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst:62
+Requires C++11 or above.
+Does not provide auto-fixes.
+

Why not? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked an inline comment as done.
PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:127-128
+
+  diag(MatchedDecl->getLocation(), "enum %0 derive from %1 of size %2 bytes, "
+   "derive from '%3' to reduce it size to %4")
+  << MatchedDecl << MatchedDecl->getIntegerType() << Size << NewType.first

unterumarmung wrote:
> I find the enum's type derivation message to be a bit unintuitive. It would 
> slightly improve the user experience if the error message provided clearer 
> information, like stating "enum %0 has a base type of %1..." or "the base 
> type of enum %0 appears excessive for its value set...". However, please 
> remember that these are merely personal thoughts, and as a non-contributor, 
> my suggestions are not obligatory.
Hmm, ok, I think I can change it to utilize a "base type" instead of "derive" 
and something like "appears excessive for its value set.".



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst:62
+Requires C++11 or above.
+Does not provide auto-fixes.
+

unterumarmung wrote:
> Why not? 
Problem is mainly with forward declarations, and a fact that some of these 
changes may be unnecessary from a domain point of view. I would prefer users to 
change enum sizes on their own risk. And I didn't wanted to add this at the 
beginning, in future maybe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL added a comment.

Option need to be changed from Regex to List


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 502790.
PiotrZSL added a comment.

Ping, Rebase, Changed Regexp option into List


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,105 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreList, value: '::IgnoredEnum;IgnoredSecondEnum'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' derive from 'int' of size 4 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' derive from 'std::int16_t' (aka 'short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' derive from 'std::uint16_t' (aka 'unsigned short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' derive from 'int' of size 4 bytes, derive from 'std::int16_t' to reduce it size to 2 [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
+
+namespace internal
+{
+
+enum class IgnoredSecondEnum
+{
+unused1 = 1,
+unused2 = 2
+};
+
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,73 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+class based on the range of its enumerators. Analyzes the values of the
+enumerators in an ``enum`` or ``enum`` class, including signed values, to
+recommend the smallest possible underlying type that can represent all the
+values of the ``enum``. The suggested underlying types are the integral types
+``std::uint8_t``, ``std::uint16_t``, and ``std::uint32_t`` for unsigned types,
+and ``std::int8_t``, ``std::int16_t``, and ``std::int32_t`` for signed types.
+Using the suggested underlying types can help reduce the memory footprint of
+the program and improve performance in some cases.
+
+For example:
+
+.. code-block:: c++
+
+// BEFORE
+enum Color {
+

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 518339.
PiotrZSL added a comment.

Ping, Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,105 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreList, value: '::IgnoredEnum;IgnoredSecondEnum'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' derive from 'int' of size 4 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' derive from 'std::int16_t' (aka 'short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' derive from 'std::uint16_t' (aka 'unsigned short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' derive from 'int' of size 4 bytes, derive from 'std::int16_t' to reduce it size to 2 [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
+
+namespace internal
+{
+
+enum class IgnoredSecondEnum
+{
+unused1 = 1,
+unused2 = 2
+};
+
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+class based on the range of its enumerators. Analyzes the values of the
+enumerators in an ``enum`` or ``enum`` class, including signed values, to
+recommend the smallest possible underlying type that can represent all the
+values of the ``enum``. The suggested underlying types are the integral types
+``std::uint8_t``, ``std::uint16_t``, and ``std::uint32_t`` for unsigned types,
+and ``std::int8_t``, ``std::int16_t``, and ``std::int32_t`` for signed types.
+Using the suggested underlying types can help reduce the memory footprint of
+the program and improve performance in some cases.
+
+For example:
+
+.. code-block:: c++
+
+// BEFORE
+enum Color {
+RED = -1,
+GREEN 

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
PiotrZSL updated this revision to Diff 497787.
PiotrZSL added a comment.
PiotrZSL updated this revision to Diff 497804.
PiotrZSL updated this revision to Diff 497826.
Eugene.Zelenko added reviewers: aaron.ballman, carlosgalvezp.
PiotrZSL marked 4 inline comments as done.
PiotrZSL updated this revision to Diff 497887.
PiotrZSL published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Removed change in list.rst that is related to 
cppcoreguidelines-avoid-capture-default-when-capturing-this.
Looks like that check have fixes but didnt mark fixes in that file.


PiotrZSL added a comment.

Fix typo in documentation.


PiotrZSL added a comment.

Fix tests on windows (they use int as base for enums, when on linux unsigned 
int is used)


PiotrZSL added a comment.

Fix review comments


PiotrZSL added a comment.

Ready for review




Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:99
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("e");
+  auto BaseType = MatchedDecl->getIntegerType().getCanonicalType();
+  if (not BaseType->isIntegerType())

Please do not use `auto` unless type is explicitly spelled in same function or 
iterator.



Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:100
+  auto BaseType = MatchedDecl->getIntegerType().getCanonicalType();
+  if (not BaseType->isIntegerType())
+return;

I don't think that literal boolean operators are part of LLVM coding guidelines.



Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:103
+
+  auto Size = Result.Context->getTypeSize(BaseType) / 8U;
+  if (1U == Size)

Ditto.



Comment at: clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp:120
+  auto NewType = getNewType(Size, MinV, MaxV);
+  if (not NewType.first or Size <= NewType.second)
+return;

Ditto.


Finds enum type definitions that could use smaller integral type as a base.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreRegexp, value: '^::IgnoredEnum$'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' derive from 'int' of size 4 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' derive from 'std::int16_t' (aka 'short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' derive from 'std::uint16_t' (aka 'unsigned short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' derive from 'int' of size 4 bytes, derive from 'std::int16_t' to reduce it size to 2 [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 498471.
PiotrZSL added a comment.

Updated documentation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreRegexp, value: '^::IgnoredEnum$'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' derive from 'int' of size 4 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' derive from 'std::int16_t' (aka 'short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' derive from 'std::uint16_t' (aka 'unsigned short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' derive from 'int' of size 4 bytes, derive from 'std::int16_t' to reduce it size to 2 [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+class based on the range of its enumerators. Analyzes the values of the
+enumerators in an ``enum`` or ``enum`` class, including signed values, to
+recommend the smallest possible underlying type that can represent all the
+values of the ``enum``. The suggested underlying types are the integral types
+``std::uint8_t``, ``std::uint16_t``, and ``std::uint32_t`` for unsigned types,
+and ``std::int8_t``, ``std::int16_t``, and ``std::int32_t`` for signed types.
+Using the suggested underlying types can help reduce the memory footprint of
+the program and improve performance in some cases.
+
+For example:
+
+.. code-block:: c++
+
+// BEFORE
+enum Color {
+RED = -1,
+GREEN = 0,
+BLUE = 1
+};
+
+std::optional color_opt;
+
+The `Color` ``enum`` uses the default under

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 498472.
PiotrZSL added a comment.

Typo in doc


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

Files:
  clang-tools-extra/clang-tidy/performance/CMakeLists.txt
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
  clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h
  clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
  clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp
@@ -0,0 +1,94 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s performance-enum-size %t -- \
+// RUN:   -config="{CheckOptions: [{key: performance-enum-size.EnumIgnoreRegexp, value: '^::IgnoredEnum$'}]}"
+
+namespace std
+{
+using uint8_t = unsigned char;
+using int8_t = signed char;
+using uint16_t = unsigned short;
+using int16_t = signed short;
+using uint32_t = unsigned int;
+using int32_t = signed int;
+}
+
+enum class Value
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'Value' derive from 'int' of size 4 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+
+enum class EnumClass : std::int16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: enum 'EnumClass' derive from 'std::int16_t' (aka 'short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported
+};
+
+enum EnumWithType : std::uint16_t
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithType' derive from 'std::uint16_t' (aka 'unsigned short') of size 2 bytes, derive from 'std::uint8_t' to reduce it size to 1 [performance-enum-size]
+{
+supported,
+supported2
+};
+
+enum EnumWithNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumWithNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+s1 = -128,
+s2 = -100,
+s3 = 100,
+s4 = 127
+};
+
+enum EnumThatCanBeReducedTo2Bytes
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumThatCanBeReducedTo2Bytes' derive from 'int' of size 4 bytes, derive from 'std::int16_t' to reduce it size to 2 [performance-enum-size]
+{
+a1 = -128,
+a2 = 0x6EEE
+};
+
+enum EnumOnlyNegative
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'EnumOnlyNegative' derive from 'int' of size 4 bytes, derive from 'std::int8_t' to reduce it size to 1 [performance-enum-size]
+{
+b1 = -125,
+b2 = -50,
+b3 = -10
+};
+
+enum CorrectU8 : std::uint8_t
+{
+c01 = 10,
+c02 = 11
+};
+
+enum CorrectU16 : std::uint16_t
+{
+c11 = 10,
+c12 = 0x
+};
+
+constexpr int getValue()
+{
+return 256;
+}
+
+
+enum CalculatedDueToUnknown1 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown1' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c21 = 10,
+c22 = getValue()
+};
+
+enum CalculatedDueToUnknown2 : unsigned int
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: enum 'CalculatedDueToUnknown2' derive from 'unsigned int' of size 4 bytes, derive from 'std::uint16_t' to reduce it size to 2 [performance-enum-size]
+{
+c31 = 10,
+c32 = c31 + 246
+};
+
+enum class IgnoredEnum : std::uint32_t
+{
+unused1 = 1,
+unused2 = 2
+};
Index: clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
@@ -0,0 +1,72 @@
+.. title:: clang-tidy - performance-enum-size
+
+performance-enum-size
+=
+
+Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+class based on the range of its enumerators. Analyzes the values of the
+enumerators in an ``enum`` or ``enum`` class, including signed values, to
+recommend the smallest possible underlying type that can represent all the
+values of the ``enum``. The suggested underlying types are the integral types
+``std::uint8_t``, ``std::uint16_t``, and ``std::uint32_t`` for unsigned types,
+and ``std::int8_t``, ``std::int16_t``, and ``std::int32_t`` for signed types.
+Using the suggested underlying types can help reduce the memory footprint of
+the program and improve performance in some cases.
+
+For example:
+
+.. code-block:: c++
+
+// BEFORE
+enum Color {
+RED = -1,
+GREEN = 0,
+BLUE = 1
+};
+
+std::optional color_opt;
+
+The `Color` ``enum`` uses the default underlying type,

[PATCH] D144135: [clang-tidy] Add performance-enum-size check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144135/new/

https://reviews.llvm.org/D144135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits