Refines the CStringLength modeling API to be pure.
Also removes the magic hypothetical parameter from the CStringChecker.
Now the getters and setters are behaving as expected.
Paths
| Differential D84979
[analyzer][NFC] Refine CStringLength modeling API Needs ReviewPublic Authored by steakhal on Jul 30 2020, 12:56 PM.
Details Summary Refines the CStringLength modeling API to be pure. Now the getters and setters are behaving as expected.
Diff Detail
Event TimelineHerald added subscribers: cfe-commits, ASDenysPetrov, Charusso and 7 others. · View Herald Transcript steakhal added a parent revision: D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting. Comment Actions It seems the Pre-merge clang-format went crazy. Comment Actions Really I am still not totally familiar how the checkers work and if it is good to have these function names. I was thinking about any checker that needs a string length information. It could get the data from CStringChecker using a "get" function or test if there is data at all. In this case it may happen that this get function modifies state or computes the length at that point? And how do the set operation work, it only stores a value computed by the calling code? And the create operation then does the computation and set together? The functions look not symmetrical, the set and create takes a MemRegion but the get takes a SVal. The create function sets the length for the passed memory region but makes no tests on it like the set function (can we call the set function from the create?). steakhal marked 2 inline comments as done. Comment ActionsFixed Artem's inline comments:
Comment Actions
Yea, naming things is pretty hard. I'm open to have a less verbose one - especially since these API functions will live under some namespace.
This patch makes sure that the get function will not modify the state (takes a const ProgramStateRef).
It tries to avoid storing the length information. E.g. if a region refers to a string literal - we don't have to store the length. (This was the previous behavior, I don't want to change that.)
CStringChecker needs a way to defer associating the length of the region.
The current implementation checks if the SVal is a GotoLabel (which is not a MemRegion). I didn't want to change its behavior in this patch.
No, it just creates a symbol, representing a cstring length. That symbol is (strictly speaking) not yet associated with the memory region.
We could call, but we need a mechanism to defer associating the length of a region. @balazske Unfortunately, I don't have satisfying answers to you, but any further improvements could be done in the future.
Revision Contents
Diff 286212 clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.h
clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h
clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
|
Just pass by value then. It's already a "Ref".