NoQ added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent &Call) {
+  Optional<SVal> RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+    return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}
----------------
I still believe you have not addressed the problem while moving the functions 
from D81718 to this patch. The caller of this function has no way of knowing 
whether the return value is the prvalue of the iterator or the glvalue of the 
iterator.

Looks like most callers are safe because they expect the object of interest to 
also be already tracked. But it's quite possible that both are tracked, say:

```lang=c++
  Container1<T> container1 = ...;
  Container2<Container1::iterator> container2 = { container1.begin() };
  container2.begin(); // ???
```

Suppose `Container1::iterator` is implemented as an object and 
`Container2::iterator` is implemented as a pointer. In this case 
`getIteratorPosition(getReturnIterator())` would yield the position of 
`container1.begin()` whereas the correct answer is the position of 
`container2.begin()`.

This problem may seem artificial but it is trivial to avoid if you simply stop 
defending your convoluted solution of looking at value classes instead of AST 
types.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:569-570
+  if (auto L = Amount.getAs<Loc>()) {
+    AmountV = State->getRawSVal(*L);
+    AmountVal = &AmountV;
   }
----------------
When does this happen?


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

https://reviews.llvm.org/D77229

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

Reply via email to