[PATCH] D144036: [clang-tidy] Add bugprone-enum-to-bool-conversion check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Such a great check, thanks! I have very minor comments, the most debatable one about the name of the check (but no strong opinion).



================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:107
+    CheckFactories.registerCheck<EnumToBoolConversionCheck>(
+        "bugprone-enum-to-bool-conversion");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
----------------
I think the name is a bit generic, do we foresee risk of conflict with other similar checks in the future? I wonder if we should narrow it down to something like `bugprone-non-zero-enum-to-bool-conversion`. WDYT?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp:23
+AST_MATCHER(EnumDecl, isCompleteAndHasNoZeroValue) {
+  const auto *Definition = Node.getDefinition();
+  return Definition and Node.isComplete() and
----------------
 Use explicit type


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp:24
+  const auto *Definition = Node.getDefinition();
+  return Definition and Node.isComplete() and
+         std::none_of(
----------------
The convention in the LLVM repo is to use traditional operators, please keep consistency.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp:27
+             Definition->enumerator_begin(), Definition->enumerator_end(),
+             [](const auto *Value) { return Value->getInitVal().isZero(); });
+}
----------------
Use explicit type


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp:72
+      << Enum;
+
+  diag(Enum->getLocation(), "enum is defined here", DiagnosticIDs::Note);
----------------
Unnecessary empty line


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:112
 
+- New :doc:`bugprone-enum-to-bool-conversion
+  <clang-tidy/checks/bugprone/enum-to-bool-conversion>` check.
----------------
Keep alphabetical order.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:12-13
+
+The check produces false positives if the ``enum`` is used to store other values
+(used as a bit-mask or zero-initialized on purpose). To deal with false-positives,
+``//NOLINT`` or casting first to the underlying type before casting to ``bool``
----------------
I can't think of a case where this could happen, might be worth adding an example below.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:14
+(used as a bit-mask or zero-initialized on purpose). To deal with false-positives,
+``//NOLINT`` or casting first to the underlying type before casting to ``bool``
+can be used.
----------------
Nit: Add space between `//` and `NOLINT`


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:38
+    void process(EStatus status)
+    {
+        if (not status)
----------------
Nit: use 2 chars indentation


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:39
+    {
+        if (not status)
+        {
----------------
Use traditional operators for consistency.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:41
+        {
+            // this true-branch in theory wont be executed
+            return;
----------------
won't


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst:41
+        {
+            // this true-branch in theory wont be executed
+            return;
----------------
carlosgalvezp wrote:
> won't
Remove?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp:9
+    SUCCESS       = 1,
+    FAILURE       = 2,
+    INVALID_PARAM = 3,
----------------
Indent with 2 chars instead of 4.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp:16
+{
+// CHECK-MESSAGES: :[[@LINE+1]]:12: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion]
+    return static_cast<bool>(value);
----------------
Nit: Add 2 chars indentation to align with the code.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp:20
+
+enum EResult : short
+{
----------------
Maybe test also "int", since it's the most common default? Also test one `enum class` without explicit underlying type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144036