vsavchenko marked an inline comment as done.
vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151
+ Optional<QualType> VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) {
+ return Visit(LCV.getRegion());
+ }
----------------
NoQ wrote:
> This is correct except you need to get the value type, not the pointer type.
>
> `LazyCompoundVal` is a `prvalue` and its parent region is the location in
> which this `prvalue` resided some time in the past. So the parent region is
> of the right type and it's always typed but you need the pointee type.
OK then, can you maybe hint how can I write a test where this is going to be a
pointer type (or maybe then `getType` for regions works incorrectly).
================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:162
+ }
+ Optional<QualType> VisitSymExpr(const SymExpr *SE) { return SE->getType(); }
+};
----------------
NoQ wrote:
> The biggest disappointment here is that this case is technically incorrect
> for the very basic case of integral types.
>
> Because we drop casts, a symbol of an integral type may technically represent
> a value of a completely different integral type, the same symbol may
> represent multiple different values of different integral types, and so on.
>
> This is what ruins the dream of modeling Region Store binding extents as
> value widths: for the single most important case we'd be getting incorrect
> answers.
I hope that the introduction of symbolic casts is around the corner.
================
Comment at: clang/unittests/StaticAnalyzer/SValTest.cpp:144
+ SVal X = getByName("x");
+ // TODO: Find a way how to get type of nonloc::ConcreteInt
+ EXPECT_FALSE(X.getType().hasValue());
----------------
NoQ wrote:
> `nonloc::ConcreteInt` is basically an `APSInt`. You can extract bit width and
> signedness and feed it to `ASTContext::getIntTypeForBitwidth()`.
>
> `loc::ConcreteInt` is always of the pointer type. Unless, of course, you have
> a target architecture with pointers of different widths, then whelp we aren't
> quite there yet. But when we get there, I guess it's still `APSInt` under the
> hood so a similar trick could be used.
OK, I guess having `ASTContext` as a parameter is the best choice we can have
here and not such a high price to pay.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104550/new/
https://reviews.llvm.org/D104550
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
