[PATCH] D70320: [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val) {
   if (auto Reg = Val.getAsRegion()) {
     Reg = Reg->getMostDerivedObjectRegion();
-    return State->get<IteratorRegionMap>(Reg);
+    return State->remove<IteratorRegionMap>(Reg);
   } else if (const auto Sym = Val.getAsSymbol()) {
-    return State->get<IteratorSymbolMap>(Sym);
+    return State->remove<IteratorSymbolMap>(Sym);
   } else if (const auto LCVal = Val.getAs<nonloc::LazyCompoundVal>()) {
----------------
baloghadamsoftware wrote:
> Charusso wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Maybe move this function to `Iterator.cpp` as well, and then move the definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which will allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and additionally guarantee that all access to the maps goes through the accessor methods that you provide?
> > > Hmm, I was trying hard to use these macros but failed so I reverted to the manual solution. I will retry now.
> > Here is a how-to: https://reviews.llvm.org/D69726
> > 
> > You need to add the fully qualified names to the register macro because of the global scope. I hope it helps.
> OK, I checked it now. If we want to put the maps into `Iterator.cpp` then we also have to move a couple of functions there which are only used by the modeling part: the internals of `checkDeadSymbols()` and `checkLiveSymbols()` must go there, although no other checker should use them. Also `processIteratorPositions()` iterates over these maps, thus it must also go there. Should I do it? Generally I like the current solution better, only functions used by multiple checker classes are in the library.
> 
> On the other hand I do not see why I should move `assumeNoOverflow()` to `Iterator.cpp`? This function is only used by the modeling part, and does not refer to the maps. You mean that we should ensure this constraint whenever setting the position for any iterator? This would mean that we should decompose every symblic expression and (re)assume this range on the symbolic part. Or we should replace `setPosition()` by at least two different functions (e.g. `setAdvancedPosition()` and `setConjuredPosition()`) but this means a total reqriting of the modeling.
> 
> I plan some refactoring but this first patch is meant to be a single separation of code.
@Charusso It does not help because the macros put the maps into a local namespace. If it is in a header, it means separate maps for every checker and the modeling which of course does not work.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:1308-1318
+ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val) {
   if (auto Reg = Val.getAsRegion()) {
     Reg = Reg->getMostDerivedObjectRegion();
-    return State->get<IteratorRegionMap>(Reg);
+    return State->remove<IteratorRegionMap>(Reg);
   } else if (const auto Sym = Val.getAsSymbol()) {
-    return State->get<IteratorSymbolMap>(Sym);
+    return State->remove<IteratorSymbolMap>(Sym);
   } else if (const auto LCVal = Val.getAs<nonloc::LazyCompoundVal>()) {
----------------
baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > Charusso wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > Maybe move this function to `Iterator.cpp` as well, and then move the definitions for iterator maps from `Iterator.h` to `Iterator.cpp`, which will allow you to use the usual `REGISTER_MAP_WITH_PROGRAMSTATE` macros, and additionally guarantee that all access to the maps goes through the accessor methods that you provide?
> > > > Hmm, I was trying hard to use these macros but failed so I reverted to the manual solution. I will retry now.
> > > Here is a how-to: https://reviews.llvm.org/D69726
> > > 
> > > You need to add the fully qualified names to the register macro because of the global scope. I hope it helps.
> > OK, I checked it now. If we want to put the maps into `Iterator.cpp` then we also have to move a couple of functions there which are only used by the modeling part: the internals of `checkDeadSymbols()` and `checkLiveSymbols()` must go there, although no other checker should use them. Also `processIteratorPositions()` iterates over these maps, thus it must also go there. Should I do it? Generally I like the current solution better, only functions used by multiple checker classes are in the library.
> > 
> > On the other hand I do not see why I should move `assumeNoOverflow()` to `Iterator.cpp`? This function is only used by the modeling part, and does not refer to the maps. You mean that we should ensure this constraint whenever setting the position for any iterator? This would mean that we should decompose every symblic expression and (re)assume this range on the symbolic part. Or we should replace `setPosition()` by at least two different functions (e.g. `setAdvancedPosition()` and `setConjuredPosition()`) but this means a total reqriting of the modeling.
> > 
> > I plan some refactoring but this first patch is meant to be a single separation of code.
> @Charusso It does not help because the macros put the maps into a local namespace. If it is in a header, it means separate maps for every checker and the modeling which of course does not work.
@NoQ What is the decision? Should I move everything to the lib such as the body of `checkDeadSymbols()` and `checkLiveSymbols()` which I am reluctant because they belong only to the modeling, or leave it as it is now?


Repository:
  rC Clang

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

https://reviews.llvm.org/D70320