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

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 4 inline comments as done.
gamesh411 added a comment.

In D153954#4456713 <https://reviews.llvm.org/D153954#4456713>, @shafik wrote:

> I did not look at this in detail but I don't think this approach is correct. I fixed this for constant evaluation the other day and you can find the details here: D130058 <https://reviews.llvm.org/D130058>
>
> The test suite I wrote should be sufficient for you to validate your approach against.

@shafik, @donat.nagy  Thanks for looking at this patch.
I have checked the linked improvements in Sema, and this initial modification would only lead to a ClangSA implementation of the same error-detection logic.
Due to symbolic execution, the set of potentially detectable errors is bigger, but this is maybe not the right place for this checker.

My goal is to improve the checker and eventually bring it out of alpha.

I see one main problem with this checker:

It does not support enums with fixed underlying type, namely `std::byte`, which is implemented like this:

  enum class byte: unsigned char {};

As `std::byte` has no enumerators, the checker would say any otherwise allowed non-default initialization of a std::byte instance is bad, i.e.:

  std::byte b {42}; // the checker gives a warning for this

Aside from this, in the `optin` package, there is a place for the current state of the checker (namely that only the value mentioned in the enumerator list are OK).
I will create a separate patch to move it out of alpha and into `optin` package.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:42
+class RangeBasedValueMatchEvaluator : ValueMatchEvaluator {
+  llvm::APSInt Min, Max;
 
----------------
steakhal wrote:
> I can see `llvm::APSInt` used a few places. Consider `using namespace llvm;`
Again, good point, I just abandoned the direction where many APSInt mentions are introduced, and the original only has 2 mentions of APSInt.


================
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);
+      }
----------------
steakhal wrote:
> 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.
Thanks! This is a very good point; I just changed the general direction of the checker, and this implementation is no longer touched by the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954