[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D153954#4480103 <https://reviews.llvm.org/D153954#4480103>, @donat.nagy wrote:

> I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest keeping the old behavior in the general case, eliminating the "obvious" false positives like `std::byte` (don't report types that have no enumerators) and moving this checker towards the optin group.

I would agree that after ignoring the obvious cases (like no enumerators), a checker like this could be useful as an "optin" checker, however, I'd not recommend moving the actual implementation anywhere.
You can create "virtual" checkers like it's done in many cases, eg. `MismatchedDeallocatorChecker`.

FYI I haven't checked the actual implementation thoroughly, because I had higher-level remarks inline.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:42
+class RangeBasedValueMatchEvaluator : ValueMatchEvaluator {
+  llvm::APSInt Min, Max;
 
----------------
I can see `llvm::APSInt` used a few places. Consider `using namespace llvm;`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:65-69
+      if (TrueState && !FalseState) {
+        Pred = C.addTransition(TrueState, Pred);
+      } else if (FalseState && !TrueState) {
+        Pred = C.addTransition(FalseState, Pred);
+      }
----------------
Why do you fold the "Pred" ExplodedNode?
I'd say, you probably didn't want to use `addTransition` here.
Why don't you assume the subsequent assumptions and transition only once?

Anyway, I think it's better to have the `addTransition` closer to the outer scope (where the error reporting is done), so that we can easily see how many ways we branch off, etc.


================
Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:44
+    // Suppress unused warnings
+    [](...){}(unscoped, scoped);
+  }
----------------
`void sink(...);` would have achieved the same and I'd say it's less complex.
`sink(unscoped, scoped);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954