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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
