baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:56
+ SVal RetVal) const;
+ void handleErase(CheckerContext &C, const Expr *CE, SVal Cont, SVal Iter,
+ SVal RetVal) const;
----------------
martong wrote:
> I understand that the container SVal is no longer `const`, but why did the
> iterator loose its constness?
We passed it earlier as a constant reference, but `SVal` is small, it should be
passed by value.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:277
void ContainerModeling::handleBegin(CheckerContext &C, const Expr *CE,
- const SVal &RetVal, const SVal &Cont) const
{
+ SVal RetVal, SVal Cont) const {
const auto *ContReg = Cont.getAsRegion();
----------------
martong wrote:
> Why do we skip the `const` qualifiers here?
Do you mean for `SVal`? See above.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:436
+ if (!StateEmpty && !StateNonEmpty) {
+ C.generateSink(State, C.getPredecessor());
+ return;
----------------
martong wrote:
> Is there really no sense to continue the analysis here? In what state we are
> here exactly? Is there an inconsistency between the `begin` , `end` and the
> return value?
Yes, see the comment I added. If data about emptiness based on the difference
of begin and and and the return value of `empty()` contradict each other then
we are on an impossible branch.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:19
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+ SymbolRef Sym2, bool Equal);
----------------
martong wrote:
> Docs, docs, docs.
This function is moved here, it is nothing new.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:274
+assumeComparison(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
+ DefinedSVal RetVal, OverloadedOperatorKind Op) {
+ if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
----------------
martong wrote:
> Perhaps we should guard `Op` with an assert, I mean it should be either == or
> !=.
Actually, this works for any comparison operator. So it is enough to assume
that `Op` is a comparison operator.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:290
+ if (StateTrue)
+ StateTrue = StateTrue->assume(RetVal, true);
+ if (StateFalse)
----------------
martong wrote:
> Why do we `assume` here again? We already had an assume in `relateSymbols`,
> hadn't we?
In `relateSymbols` we assumed the comparison of the two symbols. Here we assume
the `RetVal`. The new states are `nullptr` if we cannot assume the same for
both.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:297
+
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+ SymbolRef Sym2, bool Equal) {
----------------
martong wrote:
> Would `assumeEqual` be a better name?
This method is just moved from another source file.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:301
+
+ // FIXME: This code should be reworked as follows:
+ // 1. Subtract the operands using evalBinOp().
----------------
martong wrote:
> Please explain in the comment what is the problem with the current impl.
Ditto. The community requested a future refactoring.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:313-314
+
+ auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+ if (!NewState)
+ return nullptr;
----------------
martong wrote:
> Could we merge the declaration of the new state into the parens of the `if`?
No, because we use it later in the function. (See below.)
================
Comment at: clang/test/Analysis/container-modeling.cpp:19
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+ unsigned int __line, __const char *__function)
----------------
martong wrote:
> It seems like we don't use it anywhere.
We use this in the definition of the `assert()` macro. And we use that macro in
the new test cases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76590/new/
https://reviews.llvm.org/D76590
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
