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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

> Why don't you use the `SValVisitor` instead?

I simply didn't know of its exsistence. We can try to transform this patch using `SValVisitor` in the next revision to make the review easier and avoid additional complexity.



================
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);
----------------
steakhal wrote:
> Why are all of these `public`?
> I would expect only the first overload to be public.
Great catch. I'll do it.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650
+  // Array to pointer.
+  if (isa<ArrayType>(OriginalTy))
+    if (CastTy->isPointerType() || CastTy->isReferenceType())
       return UnknownVal();
----------------
steakhal wrote:
> Arrays decay to a pointer to the first element, but in this case, you return `Unknown`.
> Why?
This is just how `evalCast` worked before the patch. I didn't think of why. I've just tried to replicate previous cast logic. If I'd change anything, you'd catch a bunch of differences in CodeChecker. That's what I didn't want the most.



================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:680-681
+
+  // Try to cast to array
+  const auto *arrayT = dyn_cast<ArrayType>(OriginalTy.getCanonicalType());
+
----------------
steakhal wrote:
> nit
Thanks! I'll check all for the naming rules.


================
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;
+  };
----------------
steakhal wrote:
> Just call immediately that lambda and assign that value to a `const llvm::APSInt CastedValue`.
I just wanted to make this lazy. Otherwise this set of calls will be invoked unnecessarily in some cases. I'd prefer to leave it as it is.


================
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) ||
----------------
steakhal wrote:
> Presumably what?
We assume that this condition presents a type which is opposite to `float` (aka `non-float`). But it may happen that it is also opposite to some other types. So naming it `non-float` could be a bit incomplete and we are dealing with some other type. Yes, I feel this embarrassing. I'll remove `(presumably)`.


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

https://reviews.llvm.org/D90157