Szelethus added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+        BR.markInteresting(It1);
+        if (const auto &LCV1 = It1.getAs<nonloc::LazyCompoundVal>()) {
+          BR.markInteresting(LCV1->getRegion());
+        }
----------------
NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > I'm opposed to this code for the same reason that i'm opposed to it in 
> > > the debug checker. Parent region is an undocumented implementation detail 
> > > of `RegionStore`. It is supposed to be immaterial to the user. You should 
> > > not rely on its exact value.
> > > 
> > > @baloghadamsoftware Can we eliminate all such code from iterator checkers 
> > > and instead identify all iterators by regions in which they're stored? 
> > > Does my improved C++ support help with this anyhow whenever it kicks in?
> > How to find the region where it is stored? I am open to find better 
> > solutions, but it was the only one I could find so far. If we ignore 
> > `LazyCompoundVal` then everything falls apart, we can remove all the 
> > iterator-related checkers.
> It's the region from which you loaded it. If you obtained it as 
> `Call.getArgSVal()` then it's the parameter region for the call. If you 
> obtained it as `Call.getReturnValue()` then it's the target region can be 
> obtained by looking at the //construction context// for the call.
`LazyCompoundVal` and friends seem to be a constantly emerging headache for 
almost everyone. For how long I've spent in the analyzer, and have religiously 
studied conversations and your workbook about it, I still feel anxious whenever 
it comes up.

It would be great to have this documented in the code sometime.


================
Comment at: clang/test/Analysis/iterator-modelling.cpp:169
 
   clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &v); // 
expected-warning{{TRUE}}
                                                                     // 
expected-note@-1{{TRUE}}
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > Interestingness won't be propagated here because 
> > `clang_analyzer_iterator_container(i2) == &v` is interesting, not 
> > `clang_analyzer_iterator_container(i2)`, correct?
> Currently only `clang_analyzer_express()` marks its argument as interesting. 
> This could be extended in the future, however the argument of 
> `clang_analyzer_eval()` is usually the result of the comparison, not the 
> symbolic comparison itself so the sides of the comparison are not reachable 
> at that point.
Fair enough.


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

https://reviews.llvm.org/D75677



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to