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

stevewan added a comment. In D104550#3139684 , @martong wrote: > In D104550#3139239 , @stevewan > wrote: > >> In D104550#2849582 , @vsavchenko >> wrote: >> >>> In

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

martong added a comment. In D104550#3139239 , @stevewan wrote: > In D104550#2849582 , @vsavchenko > wrote: > >> In D104550#2849561 , >> @DavidSpickett wrote: >> >>>

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

stevewan added a comment. In D104550#2849582 , @vsavchenko wrote: > In D104550#2849561 , @DavidSpickett > wrote: > >> @vsavchenko One of the added tests is failing on our 32 bit Armv7 Thumb bot: >>

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

DavidSpickett added a comment. Cool. If you see a few more emails with the same thing ignore them, the armv7 bots are rather slow to catch up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550

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

vsavchenko added a comment. In D104550#2849561 , @DavidSpickett wrote: > @vsavchenko One of the added tests is failing on our 32 bit Armv7 Thumb bot: > https://lab.llvm.org/buildbot/#/builders/170/builds/61 > > >

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

DavidSpickett added a comment. @vsavchenko One of the added tests is failing on our 32 bit Armv7 Thumb bot: https://lab.llvm.org/buildbot/#/builders/170/builds/61 /home/tcwg-buildslave/worker/clang-thumbv7-full-2stage/llvm/clang/unittests/StaticAnalyzer/SValTest.cpp:169: Failure Expected

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG159024ce2315: [analyzer] Implement getType for SVal (authored by vsavchenko). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

vsavchenko updated this revision to Diff 355152. vsavchenko added a comment. Add one more note to `getType` docstring Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550 Files:

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

Szelethus accepted this revision. Szelethus added a comment. Really appreciate the unit tests! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550 ___ cfe-commits

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

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Amazing, thanks!! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:213 + /// bound expression AST node as well, since AST always has exact types. +

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

vsavchenko updated this revision to Diff 354475. vsavchenko marked 9 inline comments as done. vsavchenko added a comment. Address comments from review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550

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

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:154 + Optional VisitLocGotoLabel(loc::GotoLabel GL) { +return QualType{Context.VoidPtrTy}; + } NoQ wrote: > ASDenysPetrov wrote: > > vsavchenko wrote: > > >

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:154 + Optional VisitLocGotoLabel(loc::GotoLabel GL) { +return QualType{Context.VoidPtrTy}; + } ASDenysPetrov wrote: > vsavchenko wrote: > > ASDenysPetrov wrote: > > > I'm not

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

ASDenysPetrov added a comment. > 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. My fault. For some reason I thought you added a QualType as a member to SVal declaration. > I

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

Szelethus added a comment. In D104550#2838845 , @vsavchenko wrote: >> 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,

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

vsavchenko added a comment. In 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

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

ASDenysPetrov added a comment. 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. Since `sizeof(SVal)` became bigger (x1.5). I'm wondering of how much

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

NoQ added a comment. Another thing to mention about `SVal::getType()` in its doxygen comment is that //most of the time you don't need it//. Similarly to how checking whether an `SVal` is a `Loc` or a `NonLoc` results in incorrect code 95% of the time (because such code is unable to

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

Szelethus added a comment. In D104550#2833020 , @vsavchenko wrote: > Do you think that "for the sake of incremental approach"-like comment can > help? A `TODO:` should be good enough for now. I see this is still a moving target. Repository: rG

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

vsavchenko added a comment. In D104550#2832983 , @Szelethus wrote: > This is not an urgent, but still a very important issue: `SVal` is likely > among the first things a new developer in the analyzer sees. Its common > around the university here that

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

Szelethus added a comment. This is not an urgent, but still a very important issue: `SVal` is likely among the first things a new developer in the analyzer sees. Its common around the university here that some students barely halfway through their BSc are given smaller tasks, like the

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151 + Optional VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) { +return Visit(LCV.getRegion()); + } vsavchenko wrote: > NoQ wrote: > > vsavchenko wrote: > > > NoQ

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

vsavchenko marked an inline comment as done. vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151 + Optional VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) { +return Visit(LCV.getRegion()); + } NoQ wrote: >

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151 + Optional VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) { +return Visit(LCV.getRegion()); + } vsavchenko wrote: > NoQ wrote: > > This is correct except you need

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

vsavchenko marked an inline comment as done. vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151 + Optional VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) { +return Visit(LCV.getRegion()); + } NoQ wrote: >

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

vsavchenko updated this revision to Diff 353371. vsavchenko added a comment. Support ConcreteInt, LocAsInteger, and GotoLabel Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550 Files:

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

vsavchenko updated this revision to Diff 353343. vsavchenko added a comment. Support GotoLabel Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104550/new/ https://reviews.llvm.org/D104550 Files:

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

NoQ added a comment. Excellent! This utility is the first step on a lot of paths such as: - Asserting that all expressions' values are of the right type. I expect this to uncover a lot of ridiculous mutually cancelling bugs. - Modeling extents of RegionStore bindings - they're simply widths of

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

vsavchenko added a comment. Hey folks! I was thinking that this method can be quite handy. I think it would be great to have another pair of eyes (but more would be better) to look into this and: - Suggest other test cases to add - See if I missed any "typed" values from the hierarchy - See if

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

vsavchenko created this revision. vsavchenko added reviewers: NoQ, xazax.hun, martong, steakhal, Szelethus, ASDenysPetrov, manas, RedDocMD. Herald added subscribers: dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, mgorny. vsavchenko requested review of this