19n07u5 added a comment. The title is a little bit confusing because only the C-string size model is going to be separated and be accessible. 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`.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:146 + CStringChecker + ) ---------------- 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. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43 + ProgramStateRef &State, const Expr *Ex, + SVal Buf, bool Hypothetical = false); + ---------------- 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84316/new/ https://reviews.llvm.org/D84316 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
