[PATCH] D26588: Add LocationContext to members of check::RegionChanges

Artem Dergachev via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 14 11:37:31 PST 2016


NoQ added a comment.

Welcome to phabricator! I agree that having the location context in this callback is useful, and i'm all for reducing boilerplate in various checkers through better API.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:231
 
   ProgramStateRef bindLoc(Loc location,
                           SVal V,
----------------
Because this API becomes more complicated, i think we should add some docstrings here, explaining the meaning of the location context parameter, in particular.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:733
+inline SVal ProgramState::getArgSVal(const StackFrameContext *SFC,
+                              const unsigned ArgIdx) const {
+  const FunctionDecl *FunctionDecl = SFC->getDecl()->getAsFunction();
----------------
Indent.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:741
+    // because the call wasn't modeled in the first place.
+    const VarDecl *ArgDecl = FunctionDecl->parameters()[ArgIdx];
+    const Loc ArgLoc = getLValue(ArgDecl, SFC);
----------------
a.sidorin wrote:
> Unfortunately, this code does not consider the fact that argument values may be overwritten. If we want to get initial values, we should find another way.
Uhm, yeah, this is in fact a problem!

The purpose of this code is, in fact, to construct `SymbolRegionValue` for the parameter, so it is equivalent to calling `SValBuilder::getRegionValueSymbolVal()` over the parameter region, as long as the current `StoreManager` implementation remains unquestioned.

We could also ask `StoreManager` to provide a binding for this region from an empty store, maybe extend its API to allow such queries, this would remove the layering violation.

Because this topic getting is rather complicated, it might have been better to make a separate review for this change, originally (no problem now that most of the interested people have already had a look).


https://reviews.llvm.org/D26588





More information about the cfe-commits mailing list