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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
