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