[clang-tidy] Add performance-enum-size check

[PiotrZSL]

Summary

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

[PiotrZSL]

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]

Fix typo in documentation.

[PiotrZSL]

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

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp

100

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

101

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

104

Ditto.

121

Ditto.

[PiotrZSL]

Fix review comments

[PiotrZSL]

Ready for review

[PiotrZSL]

Updated documentation.

[PiotrZSL]

Typo in doc

[PiotrZSL]

Option need to be changed from Regex to List

[PiotrZSL]

Ping, Rebase, Changed Regexp option into List

[PiotrZSL]

Ping, Rebase

[PiotrZSL]

Ping

[PiotrZSL]

Ping

[unterumarmung]

clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp

128–129

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.

clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst

63

Why not?

[PiotrZSL]

clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp

128–129

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.".

clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst

63

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.

[xgupta]

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

[PiotrZSL]

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

[Eugene.Zelenko]

In D144135#4524902, @PiotrZSL wrote:

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

I'm sorry for noise.

[PiotrZSL]

Rebase + Update diagnostic message

[xgupta]

LGTM, thanks.

clang-tools-extra/clang-tidy/performance/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyPerformanceModule
   AvoidEndlCheck.cpp
+  EnumSizeCheck.cpp
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitConversionInLoopCheck.cpp
   InefficientAlgorithmCheck.cpp
   InefficientStringConcatenationCheck.cpp
   InefficientVectorOperationCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp

clang-tools-extra/clang-tidy/performance/EnumSizeCheck.h

+//===--- EnumSizeCheck.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_PERFORMANCE_ENUMSIZECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_ENUMSIZECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <vector>
+
+namespace clang::tidy::performance {
+
+/// Finds `enum` type definitions that could use smaller integral type as a
+/// base.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/enum-size.html
+class EnumSizeCheck : public ClangTidyCheck {
+public:
+  EnumSizeCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+
+private:
+  const std::vector<StringRef> EnumIgnoreList;
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_ENUMSIZECHECK_H

clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp

+//===--- EnumSizeCheck.cpp - clang-tidy -----------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "EnumSizeCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <algorithm>
+#include <cinttypes>
+#include <cstdint>
+#include <limits>
+#include <utility>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+namespace {
+
+const std::uint64_t Min8 =
+    std::imaxabs(std::numeric_limits<std::int8_t>::min());
+const std::uint64_t Max8 = std::numeric_limits<std::int8_t>::max();
+const std::uint64_t Min16 =
+    std::imaxabs(std::numeric_limits<std::int16_t>::min());
+const std::uint64_t Max16 = std::numeric_limits<std::int16_t>::max();
+const std::uint64_t Min32 =
+    std::imaxabs(std::numeric_limits<std::int32_t>::min());
+const std::uint64_t Max32 = std::numeric_limits<std::int32_t>::max();
+
+std::pair<const char *, std::uint32_t>
+getNewType(std::size_t Size, std::uint64_t Min, std::uint64_t Max) noexcept {
+  if (Min) {
+    if (Min <= Min8 && Max <= Max8) {
+      return {"std::int8_t", sizeof(std::int8_t)};
+    }
+
+    if (Min <= Min16 && Max <= Max16 && Size > sizeof(std::int16_t)) {
+      return {"std::int16_t", sizeof(std::int16_t)};
+    }
+
+    if (Min <= Min32 && Max <= Max32 && Size > sizeof(std::int32_t)) {
+      return {"std::int32_t", sizeof(std::int32_t)};
+    }
+
+    return {};
+  }
+
+  if (Max) {
+    if (Max <= std::numeric_limits<std::uint8_t>::max()) {
+      return {"std::uint8_t", sizeof(std::uint8_t)};
+    }
+
+    if (Max <= std::numeric_limits<std::uint16_t>::max() &&
+        Size > sizeof(std::uint16_t)) {
+      return {"std::uint16_t", sizeof(std::uint16_t)};
+    }
+
+    if (Max <= std::numeric_limits<std::uint32_t>::max() &&
+        Size > sizeof(std::uint32_t)) {
+      return {"std::uint32_t", sizeof(std::uint32_t)};
+    }
+
+    return {};
+  }
+
+  // Zero case
+  return {"std::uint8_t", sizeof(std::uint8_t)};
+}
+
+} // namespace
+
+EnumSizeCheck::EnumSizeCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      EnumIgnoreList(
+          utils::options::parseStringList(Options.get("EnumIgnoreList", ""))) {}
+
+void EnumSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "EnumIgnoreList",
+                utils::options::serializeStringList(EnumIgnoreList));
+}
+
+bool EnumSizeCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
+
+void EnumSizeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      enumDecl(unless(isExpansionInSystemHeader()), isDefinition(),
+               unless(matchers::matchesAnyListedName(EnumIgnoreList)))
+          .bind("e"),
+      this);
+}
+
+void EnumSizeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<EnumDecl>("e");
+  const QualType BaseType = MatchedDecl->getIntegerType().getCanonicalType();
+  if (!BaseType->isIntegerType())
+    return;
+
+  const std::uint32_t Size = Result.Context->getTypeSize(BaseType) / 8U;
+  if (1U == Size)
+    return;
+
+  std::uint64_t MinV = 0U;
+  std::uint64_t MaxV = 0U;
+
+  for (const auto &It : MatchedDecl->enumerators()) {
+    const llvm::APSInt &InitVal = It->getInitVal();
+    if ((InitVal.isUnsigned() || InitVal.isNonNegative())) {
+      MaxV = std::max<std::uint64_t>(MaxV, InitVal.getZExtValue());
+    } else {
+      MinV = std::max<std::uint64_t>(MinV, InitVal.abs().getZExtValue());
+    }
+  }
+
+  auto NewType = getNewType(Size, MinV, MaxV);
+  if (!NewType.first || Size <= NewType.second)
+    return;
+
+  diag(MatchedDecl->getLocation(),
+       "enum %0 uses a larger base type (%1, size: %2 %select{byte|bytes}5) "
+       "than necessary for its value set, consider using '%3' (%4 "
+       "%select{byte|bytes}6) as the base type to reduce its size")
+      << MatchedDecl << MatchedDecl->getIntegerType() << Size << NewType.first
+      << NewType.second << (Size > 1U) << (NewType.second > 1U);
+}
+
+} // namespace clang::tidy::performance

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

 //===-- PerformanceTidyModule.cpp - clang-tidy ----------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidEndlCheck.h"
+#include "EnumSizeCheck.h"
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitConversionInLoopCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "InefficientStringConcatenationCheck.h"
 #include "InefficientVectorOperationCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"

 
 namespace clang::tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidEndlCheck>("performance-avoid-endl");
+    CheckFactories.registerCheck<EnumSizeCheck>("performance-enum-size");
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "performance-faster-string-find");
     CheckFactories.registerCheck<ForRangeCopyCheck>(
         "performance-for-range-copy");
     CheckFactories.registerCheck<ImplicitConversionInLoopCheck>(
         "performance-implicit-conversion-in-loop");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "performance-inefficient-algorithm");

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`performance-enum-size
+  <clang-tidy/checks/performance/enum-size>` check.
+
+  Recommends the smallest possible underlying type for an ``enum`` or ``enum``
+  class based on the range of its enumerators.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Removed checks
 ^^^^^^^^^^^^^^

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `objc-missing-hash <objc/missing-hash.html>`_,
    `objc-nsdate-formatter <objc/nsdate-formatter.html>`_,
    `objc-nsinvocation-argument-lifetime <objc/nsinvocation-argument-lifetime.html>`_, "Yes"
    `objc-property-declaration <objc/property-declaration.html>`_, "Yes"
    `objc-super-self <objc/super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp/exception-escape.html>`_,
    `openmp-use-default-none <openmp/use-default-none.html>`_,
    `performance-avoid-endl <performance/avoid-endl.html>`_, "Yes"
+   `performance-enum-size <performance/enum-size.html>`_,
    `performance-faster-string-find <performance/faster-string-find.html>`_, "Yes"
    `performance-for-range-copy <performance/for-range-copy.html>`_, "Yes"
    `performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop.html>`_,
    `performance-inefficient-algorithm <performance/inefficient-algorithm.html>`_, "Yes"
    `performance-inefficient-string-concatenation <performance/inefficient-string-concatenation.html>`_,
    `performance-inefficient-vector-operation <performance/inefficient-vector-operation.html>`_, "Yes"
    `performance-move-const-arg <performance/move-const-arg.html>`_, "Yes"
    `performance-move-constructor-init <performance/move-constructor-init.html>`_,

clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst

+.. 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> color_opt;
+
+The `Color` ``enum`` uses the default underlying type, which is ``int`` in this
+case, and its enumerators have values of -1, 0, and 1. Additionally, the
+``std::optional<Color>`` object uses 8 bytes due to padding (platform
+dependent).
+
+.. code-block:: c++
+
+    // AFTER
+    enum Color : std:int8_t {
+        RED = -1,
+        GREEN = 0,
+        BLUE = 1
+    }
+
+    std::optional<Color> color_opt;
+
+
+In the revised version of the `Color` ``enum``, the underlying type has been
+changed to ``std::int8_t``. The enumerator `RED` has a value of -1, which can
+be represented by a signed 8-bit integer.
+
+By using a smaller underlying type, the memory footprint of the `Color`
+``enum`` is reduced from 4 bytes to 1 byte. The revised version of the
+``std::optional<Color>`` object would only require 2 bytes (due to lack of
+padding), since it contains a single byte for the `Color` ``enum`` and a single
+byte for the ``bool`` flag that indicates whether the optional value is set.
+
+Reducing the memory footprint of an ``enum`` can have significant benefits in
+terms of memory usage and cache performance. However, it's important to
+consider the trade-offs and potential impact on code readability and
+maintainability.
+
+Requires C++11 or above.
+Does not provide auto-fixes.
+
+Options
+-------
+
+.. option:: EnumIgnoreList
+
+    Option is used to ignore certain enum types. It accepts a
+    semicolon-separated list of (fully qualified) enum type names or regular
+    expressions that match the enum type names. The default value is an empty
+    string, which means no enums will be ignored.

clang-tools-extra/test/clang-tidy/checkers/performance/enum-size.cpp

+// 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 = 0xFFFF
+};
+
+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
+};
+
+}