[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33672#819683, @xazax.hun wrote:

> Aaron, could you comment on the applicability of this check to C? Thanks in advance.


C has different rules for their enumerations in that the enumerators are all ints, but the enumeration type is either `char`, a signed integer type, or an unsigned integer type depending on the values of the enumerators. So this problem exists:

  enum E {
    one = 1
  };
  
  void f(int i) {
    enum E e = (enum E)i;
  }
  
  int main(void) {
    f(1024);
  }

If `enum E` is represented by a `char`, then the cast causes a loss of precision. However, this isn't undefined behavior in C like it is in C++.



================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:29
+namespace {
+// This evaluator checks 2 SVals for equality. The first SVal is provided via
+// the constructor, the second is the parameter of the overloaded () operator.
----------------
s/2/two


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:34
+class ConstraintBasedEQEvaluator {
+private:
+  const DefinedOrUnknownSVal CompareValue;
----------------
No need for the access specifier; it defaults to `private`.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:47
+    DefinedOrUnknownSVal EnumDeclValue = SVB.makeIntVal(EnumDeclInitValue);
+    const auto ElemEqualsValueToCast =
+        SVB.evalEQ(PS, EnumDeclValue, CompareValue);
----------------
Please do not use `auto` here as the type is not spelled out in the initialization. Also, you can drop the `const` qualifier if it's not a pointer or reference type.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:74-75
+  EnumValueVector DeclValues;
+  for (const auto *D : ED->decls()) {
+    const auto ECD = dyn_cast<EnumConstantDecl>(D);
+    DeclValues.push_back(ECD->getInitVal());
----------------
Instead of enumerating over `decls()` and then casting, just enumerate over `enumerators()` and  the cast isn't needed. Or, even better:
```
EnumValueVector DeclValues(ED->enumerator_end() - ED->enumerator_begin());
std::transform(ED->enumerator_begin(), ED->enumerator_end(), DeclValues.begin(),
                       [](const EnumConstantDecl *D) { return D->getInitVal(); });
```


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:84
+void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const {
+  if (auto N = C.generateNonFatalErrorNode(C.getState())) {
+    if (!EnumValueCastOutOfRange)
----------------
NoQ wrote:
> `C.getState()` is the default value (if you see how `generateNonFatalErrorNode()` calls `addTransition()` which in turns substitutes nullptr with `getState()`), so it can be omitted.
Don't use `auto` here.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:87-89
+          new BuiltinBug(this, "Enum cast out of range",
+                         "The value provided to the cast expression is not in "
+                         "the valid range of values for the enum."));
----------------
Also, diagnostics should not be full sentences or grammatically correct, so drop the capitalization and full stop.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:116
+  // function to handle this.
+  const EnumDecl *ED = T->getAs<EnumType>()->getDecl();
+
----------------
You can use `castAs<>()` because you've already determined it's an enumeral type.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121
+  bool PossibleValueMatch =
+      std::any_of(DeclValues.begin(), DeclValues.end(),
+                  ConstraintBasedEQEvaluator(C, *ValueToCastOptional));
----------------
You can use `llvm::any_of()` and pass in the container.


https://reviews.llvm.org/D33672