[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 7 inline comments as done.
baloghadamsoftware added a comment.

Thank you for your comments. Of course I plan to upload all these changes in at least three different patches. But first I needed a version that is working. However if I split this up I will have to write lots of unit tests.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+  Optional<SVal> getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+  Optional<SVal> getArgObject(unsigned Index, unsigned BlockCount) const;
+
+  Optional<SVal> getReturnObject(unsigned BlockCount) const;
----------------
NoQ wrote:
> I believe these functions don't need to be optional. There's always a parameter location and a location to return to (even if the latter is incorrect due to unimplemented construction context, it'll still be some dummy temporary region that you can use).
Do you mean we should return the original `SVal` if parameter or return value location fails? (Thus `LazyCompoundVal` in worst case?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;
----------------
NoQ wrote:
> There should be only one way to express the parameter region. Let's call this one simply `ParamRegion` or something like that, and `assert(!isa<ParmVarDecl>(D))` in the constructor of `VarRegion`.
Do you mean that we should use `ParamRegion` in every case, thus also when we have the definitioan for the function? I wonder whether it breaks too many things.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1066
+  QualType getValueType() const override {
+    return CallE->getType();
+  }
----------------
NoQ wrote:
> This should be the type of the parameter, not the return type of the call.
Yes, sorry, my mistake.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:278-279
+
+const ConstructionContext
+*CallEvent::getConstructionContext(unsigned BlockCount) const {
+  const CFGBlock *Block;
----------------
NoQ wrote:
> This function obviously does not depend on `BlockCount`. You might as well always use `0` as `BlockCount`.
> 
> The ideal solution, of course, is for `CallEvent` to keep a `CFGElementRef` from the start.
I plan to make that change in a separate patch.


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

https://reviews.llvm.org/D77229