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

Thu Jul 30 12:58:34 PDT 2020

steakhal added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+                                     ProgramStateRef &State, const Expr *Ex,
+                                     SVal Buf, bool Hypothetical = false);
+
----------------
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.

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

https://reviews.llvm.org/D84316