[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

99

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

100

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

103

Ditto.

120

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
+  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 <string>
+
+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:
+  std::string EnumIgnoreRegexp;
+};
+
+} // 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 "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 and Max <= Max8) {
+      return {"std::int8_t", sizeof(std::int8_t)};
+    }
+
+    if (Min <= Min16 and Max <= Max16 and Size > sizeof(std::int16_t)) {
+      return {"std::int16_t", sizeof(std::int16_t)};
+    }
+
+    if (Min <= Min32 and Max <= Max32 and 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() and
+        Size > sizeof(std::uint16_t)) {
+      return {"std::uint16_t", sizeof(std::uint16_t)};
+    }
+
+    if (Max <= std::numeric_limits<std::uint32_t>::max() and
+        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),
+      EnumIgnoreRegexp(Options.get("EnumIgnoreRegexp", "^$")) {}
+
+void EnumSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "EnumIgnoreRegexp", EnumIgnoreRegexp);
+}
+
+bool EnumSizeCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus11;
+}
+
+void EnumSizeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(enumDecl(unless(isExpansionInSystemHeader()),
+                              isDefinition(),
+                              unless(matchesName(EnumIgnoreRegexp)))
+                         .bind("e"),
+                     this);
+}
+
+void EnumSizeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<EnumDecl>("e");
+  auto BaseType = MatchedDecl->getIntegerType().getCanonicalType();
+  if (not BaseType->isIntegerType())
+    return;
+
+  auto 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() or 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 (not NewType.first or Size <= NewType.second)
+    return;
+
+  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
+      << NewType.second;
+}
+
+} // 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 "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"
 #include "NoAutomaticMoveCheck.h"
 #include "NoIntToPtrCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang::tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    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

- New :doc:`cppcoreguidelines-avoid-capture-default-when-capturing-this
   Warns when lambda specify a capture default and capture ``this``.
 
 - New :doc: `llvmlibc-inline-function-decl
   <clang-tidy/checks/llvmlibc/inline-function-decl>` check.
 
   Checks that all implicit and explicit inline functions in header files are
   tagged with the ``LIBC_INLINE`` macro.
 
+- New :doc:`performance-enum-size
+  <clang-tidy/checks/performance/enum-size>` check.
+
+  Finds ``enum`` type definitions that could use smaller integral type as a base.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c

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

.. csv-table::
    `objc-forbidden-subclassing <objc/forbidden-subclassing.html>`_,
    `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-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
+=====================
+
+Finds ``enum`` type definitions that could use smaller integral type as a base.
+By default all, ``enum`` and ``enum`` classes use as base, integral types of
+minimum size equal 32 bit. Since C++11 there is possibility to specify custom
+base type, and therefor reduce size of ``enum`` from four or eight bytes to one
+or two.
+
+Requires C++11 or above.
+
+Example
+-------
+
+.. code-block:: c++
+
+    enum EnumType // : std::int16_t
+    {
+        NEGATIVE_10   = -10,
+        POSITIVE_100  = 100,
+        POSITIVE_1000 = 1000
+    };
+
+    class Values
+    {
+    public:
+        EnumType first;
+        EnumType second;
+    };
+
+
+``EnumType`` will use as base ``int``, but could easily use ``std::int16_t``
+(``short``). After that operation size of ``Values`` would reduce from
+8 bytes to 4 bytes. This will also impact alignment of ``Values`` class by
+reducing it from 4 bytes to 2 bytes.
+
+Options
+-------
+
+.. option:: EnumIgnoreRegexp
+
+    Regexp used to ignore definitions of enum types that match regexp.
+    The default is `^$`.

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