NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
> I plan some refactoring but this first patch is meant to be a single
> separation of code.
Sounds great!
================
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:
> > 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?
I think all the functions for manipulating the state trait (including dead
symbol cleanup and escapes) should live in the modeling file. Like, it doesn't
mean that `checkDeadSymbols` itself should necessarily live there, but in any
case the file should provide a nice cleanup method that can be invoked from the
real `checkDeadSymbols`.
This way you can encapsulate all the implementation details in the cpp file and
only interact with it with fancy accessors.
I'm perfectly ok with delaying this work ^.^
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70320/new/
https://reviews.llvm.org/D70320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
