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