[PATCH] D13157: Teach -Wtautological-overlap-compare about enums

[Richard Trieu via cfe-commits]

rtrieu accepted this revision.
rtrieu added a comment.

Fix the comments, then it should be ready to commit.


================
Comment at: lib/Analysis/CFG.cpp:49
@@ +48,3 @@
+tryNormalizeBinaryOperator(const BinaryOperator *B) {
+  auto TryTransformToIntOrEnumConstant = [](const Expr *E) -> const Expr * {
+    E = E->IgnoreParens();
----------------
Why is this a lambda instead of a helper function?

================
Comment at: lib/Analysis/CFG.cpp:88
@@ +87,3 @@
+/// or DeclRefExprs (that have decls of type EnumConstantDecl)
+static bool areExprTypesCompatible(const Expr *A, const Expr *B) {
+  // User intent isn't clear if they're mixing int literals with enum
----------------
Use E1 and E2 instead of A and B to match the naming of decls below.

================
Comment at: lib/Analysis/CFG.cpp:98
@@ +97,3 @@
+
+  // Currently we're only given EnumConstantDecls or IntegerLiterals
+  auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl());
----------------
Change the comment to note that IntegerLiterals are handled above and only EnumConstantDecls are expected beyond this point.

================
Comment at: lib/Analysis/CFG.cpp:99-104
@@ +98,8 @@
+  // Currently we're only given EnumConstantDecls or IntegerLiterals
+  auto *C1 = cast<EnumConstantDecl>(cast<DeclRefExpr>(A)->getDecl());
+  auto *C2 = cast<EnumConstantDecl>(cast<DeclRefExpr>(B)->getDecl());
+
+  const TagDecl *E1 = TagDecl::castFromDeclContext(C1->getDeclContext());
+  const TagDecl *E2 = TagDecl::castFromDeclContext(C2->getDeclContext());
+  return E1 == E2;
+}
----------------
There's a few extra casts in here, plus some blind conversions between types.  Add your assumptions for the types in asserts.  Also, DeclContext should use cast<> to get to Decl types.  I recommend the following:

```
  assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));
  auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
  auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();

  assert(isa<EnumConstantDecl>(Decl1) && isa<EnumConstantDecl>(Decl2));
  const DeclContext *DC1 = Decl1->getDeclContext();
  const DeclContext *DC2 = Decl2->getDeclContext();

  assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2));
  return DC1 == DC2;

```


http://reviews.llvm.org/D13157