[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

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

steakhal added a comment.

What you are basically doing is a visitation on the kind of the `SVal`.
Why don't you use the `SValVisitor` instead? That way you could focus on the leaf nodes in the hierarchy, leaving you an even cleaner solution.

  class CastEvaluator : public SValVisitor<CastEvaluator, SVal> {
  public:
    CastEvaluator(QualType CastTy, QualType OriginalTy /*etc.*/)
    SVal VisitPointerToMember(nonloc::PointerToMember V) { return V; }
    SVal VisitGotoLabel(loc::GotoLabel V) { /*impl.*/ }
    /* etc. */
  };

Comments should be punctuated and the variables should start with an uppercase letter in general.
About the correctness: I think it still needs a bit more confidence. I'm gonna run this on LLVM itself and see if anything changes.

I really like where this is going. Keep up the good work.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:105-126
+  SVal evalCast(SVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UndefinedVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(UnknownVal V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(Loc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastKind(NonLoc V, QualType CastTy, QualType OriginalTy);
+  SVal evalCastSubKind(loc::ConcreteInt V, QualType CastTy,
+                       QualType OriginalTy);
----------------
Why are all of these `public`?
I would expect only the first overload to be public.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650
+  // Array to pointer.
+  if (isa<ArrayType>(OriginalTy))
+    if (CastTy->isPointerType() || CastTy->isReferenceType())
       return UnknownVal();
----------------
Arrays decay to a pointer to the first element, but in this case, you return `Unknown`.
Why?


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:680-681
+
+  // Try to cast to array
+  const auto *arrayT = dyn_cast<ArrayType>(OriginalTy.getCanonicalType());
+
----------------
nit


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:765-769
+  auto castedValue = [V, CastTy, this]() {
+    llvm::APSInt value = V.getValue();
+    BasicVals.getAPSIntType(CastTy).apply(value);
+    return value;
+  };
----------------
Just call immediately that lambda and assign that value to a `const llvm::APSInt CastedValue`.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:844-847
+  // Symbol to bool.
+  if (CastTy->isBooleanType()) {
+    // Non-float(presumably) to bool.
+    if (Loc::isLocType(OriginalTy) ||
----------------
Presumably what?


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

https://reviews.llvm.org/D90157