[PATCH] D129570: [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp:31
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<Expr>("x");
+  diag(MatchedExpr->getBeginLoc(), "enum is implictly converted to an integral")
+      << MatchedExpr->getSourceRange();
----------------
Check that this pointer is not nullptr, before dereferencing it. Typically we do it like this in all other checks:

  if (const auto* MatchedExpr = Result.Nodes.getNodeAs<Expr>("x")) {
    ...
  }

If you don't like the extra nesting you can return early.

  if (!MatchedExpr)
    return;  


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp:34
+  auto Note = diag(MatchedExpr->getBeginLoc(),
+                   "insert an explicit cast to silence this warning",
+                   DiagnosticIDs::Note);
----------------
I think this text is redundant given the fix-it note. Also, I find "silence this warning" a bit confusing, since in order to do that one would use `NOLINT`. By adding the cast one is not just "silencing the warning", one is fixing the code to be explicit.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp:45
+  } else {
+    Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)");
+  }
----------------
I don't think we can assume the type of the enum is `int`, see for example:

```
enum Foo : unsigned int
{
    a,
    b
};


void f(unsigned int);

int main()
{
    f(a);
}

```

Even if there is no explicit underlying type, isn't the underlying type implementation-defined?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp:1
+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
----------------
Please add a test using an enum with fixed underlying type that is not "int".


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp:3
+
+enum A { e1,
+         e2 };
----------------
carlosgalvezp wrote:
> Please use a consistent style
> 
>   enum A {
>     e1,
>     e2,
>   };
Please add a test to verify the C-style cast fix.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-int.cpp:3-4
+
+enum A { e1,
+         e2 };
+
----------------
Please use a consistent style

  enum A {
    e1,
    e2,
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129570