[PATCH] D104550: [analyzer] Implement getType for SVal

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D104550#2838827 <https://reviews.llvm.org/D104550#2838827>, @ASDenysPetrov wrote:

> I'm in favor of this patch. It will help simplify `SValBuilder::evalCast`, which takes an optional parameter `OriginalTy` and acts differently based on whether it has been passed or has not.

@NoQ mentioned it before that it is always better to use direct types from AST.  But the only participant who doesn't have AST is the Store, and when it calls `evalCast` it doesn't have a way to get that type.  So, in this case this function van be pretty useful.

> Since `sizeof(SVal)` became bigger (x1.5). I'm wondering of how much this reflects on memory consumption.

I'm not sure what you mean here.  If it is about this particular patch, it simply adds a non-virtual method to `SVal`, it shouldn't affect `sizeof(SVal)` at all.

> Another thing is that we can garantee returning `QualType`. I mean, we can replace `Optional` with `QualType` itself. `QualType` has a default ctor and `isNull` predicate, which is //true// when defaultly constructed.

I wanted to be more explicit here and convey to the user that the lack of type is normal.  I can change that, it won't be a problem at all.  @NoQ, @Szelethus what do you think?



================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:154
+  Optional<QualType> VisitLocGotoLabel(loc::GotoLabel GL) {
+    return QualType{Context.VoidPtrTy};
+  }
----------------
ASDenysPetrov wrote:
> I'm not sure this is a correct type. I would expect here something like: `class LabelType : public Type`.
I don't think that I fully understood what you suggest here.  Do you suggest to add a new type to `Type.h`?


================
Comment at: clang/unittests/StaticAnalyzer/SValTest.cpp:181-182
+void foo(int a, int b) {
+  int x = a;
+  int y = a + b;
+})") {
----------------
ASDenysPetrov wrote:
> Add a cast case.
Sure!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104550