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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
