[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D84316#2187372 <https://reviews.llvm.org/D84316#2187372>, @19n07u5 wrote:

> The title is a little bit confusing because only the C-string size model is going to be separated and be accessible.

Could you elaborate on why is the title not precise?
It seems that the modeling logic and the reporting logic will be separated:

- modeling will be implemented in `CStringLengthModeling.cpp`
- reporting will be implemented in `CStringChecker.cpp` (just as like it was before)

I just wanted a short (at most 80 char long) title, if you offer any better I would be pleased.

---

> Other than that as @NoQ pointed out we need lot more of these common-API-separation patches. It is a great starting point for the `CStringChecker`.

Thanks. I'm thinking about making the checker cleaner - we will see.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:146
+  CStringChecker
+  )
----------------
19n07u5 wrote:
> Other common checker functionality folders and headers do not require extra CMake support long ago. I think when we need such support, we could define it later, so that you could revert this.
It would be easier to use the `CStringLength.h` header without specifying the complete path to it.
IMO `#include "CStringChecker/CStringLength.h"` is kindof verbose compared to simply using `#include "CStringLength.h"`.
As of now, I'm sticking to this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+                                     ProgramStateRef &State, const Expr *Ex,
+                                     SVal Buf, bool Hypothetical = false);
+
----------------
19n07u5 wrote:
> steakhal wrote:
> > steakhal wrote:
> > > steakhal wrote:
> > > > balazske wrote:
> > > > > I do not like that the //get// and //set// (CStringLength) functions are not symmetrical. I (and other developers) would think that the get function returns a stored value and the set function sets it. The `getCStringLength` is more a `computeCStringLength` and additionally may manipulate the `State` too. In this form it is usable mostly only for CStringChecker. (A separate function to get the value stored in the length map should exist instead of this `Hypothetical` thing.)
> > > > > [...] get function returns a stored value and the set function sets it.
> > > > Certainly a burden to understand. It would be more appealing, but more useful?
> > > > The user would have to check and create if necessary regardless. So fusing these two functions is more like a feature.
> > > > What use case do you think of using only the query function? In other words, how can you guarantee that you will find a length for a symbol?
> > > > 
> > > > > In this form it is usable mostly only for CStringChecker. (A separate function to get the value stored in the length map should exist instead of this Hypothetical thing.)
> > > > You are right. However, I want to focus on splitting parts without modifying the already existing API reducing the risk of breaking things.
> > > > You should expect such a change in an upcoming patch.
> > > On second thought, It probably worth having a cleaner API to a slight inconvenience. If he feels like, still can wrap them.
> > > I will investigate it tomorrow.
> > I made a separate patch for cleansing this API.
> > In the D84979 now these API functions will behave as expected.
> > I (and other developers) would think that the get function returns a stored value and the set function sets it.
> Developers should not believe the getters are pure getters. As a checker-writer point of view, you do not care whether the C-string already exist or the checker creates it during symbolic execution, you only want to get the C-string.
> 
> Think about all the Static Analyzer getters as factory functions, that is the de facto standard now. For example, when you are trying to get a symbolic value with `getSVal()`, for the first occurrence of an expression no `SVal` exist, so it also creates it.
> 
> With that in mind, @steakhal, could you partially revert the renaming related refactors of D84979, please?
> [...] As a checker-writer point of view, you do not care whether the C-string already exist or the checker creates it during symbolic execution, you only want to get the C-string.
I would have agreed with you - before I made the D84979 patch.
Now I believe if the interface can be implemented //purely// then it should be done so.

> Think about all the Static Analyzer getters as factory functions, that is the de facto standard now.
We can always change them.

> For example, when you are trying to get a symbolic value with `getSVal()`, for the first occurrence of an expression no `SVal` exist, so it also creates it.
I'm not really familiar with the internals of `getSVal()`, I'm gonna definitely have on that.
IMO `getSVal()` is a different beast compared to the functions declared in this header file.

>With that in mind, @steakhal, could you partially revert the renaming related refactors of D84979, please?
I genuinly think that I'm on the right track.

If you don't mind, move further discussion about that to the corresponding revision.


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

https://reviews.llvm.org/D84316