martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:34
- void handleBegin(CheckerContext &C, const Expr *CE, const SVal &RetVal,
- const SVal &Cont) const;
- void handleEnd(CheckerContext &C, const Expr *CE, const SVal &RetVal,
- const SVal &Cont) const;
- void handleAssignment(CheckerContext &C, const SVal &Cont,
- const Expr *CE = nullptr,
- const SVal &OldCont = UndefinedVal()) const;
- void handleAssign(CheckerContext &C, const SVal &Cont) const;
- void handleClear(CheckerContext &C, const SVal &Cont) const;
- void handlePushBack(CheckerContext &C, const SVal &Cont) const;
- void handlePopBack(CheckerContext &C, const SVal &Cont) const;
- void handlePushFront(CheckerContext &C, const SVal &Cont) const;
- void handlePopFront(CheckerContext &C, const SVal &Cont) const;
- void handleInsert(CheckerContext &C, const SVal &Cont,
- const SVal &Iter) const;
- void handleErase(CheckerContext &C, const SVal &Cont, const SVal &Iter)
const;
- void handleErase(CheckerContext &C, const SVal &Cont, const SVal &Iter1,
- const SVal &Iter2) const;
- void handleEraseAfter(CheckerContext &C, const SVal &Cont,
- const SVal &Iter) const;
- void handleEraseAfter(CheckerContext &C, const SVal &Cont, const SVal &Iter1,
- const SVal &Iter2) const;
+ void handleBegin(CheckerContext &C, const Expr *CE, SVal RetVal,
+ SVal Cont) const;
----------------
Perhaps it would be easier to read these functions if we had a documentation
for their parameters. Maybe we could have some of them documented in one
comment since many seem to have similar params.
Or maybe just renaming `Cont` to `Container` would help also.
================
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;
----------------
I understand that the container SVal is no longer `const`, but why did the
iterator loose its constness?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:74
- typedef void (ContainerModeling::*NoItParamFn)(CheckerContext &,
- const SVal &) const;
+ typedef void (ContainerModeling::*NoItParamFn)(CheckerContext &, const Expr
*,
+ SVal, SVal) const;
----------------
Just a nit, if we are at counting below (one, two) then perhaps `Zero` would be
more coherent with the other typedefs?
================
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();
----------------
Why do we skip the `const` qualifiers here?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:436
+ if (!StateEmpty && !StateNonEmpty) {
+ C.generateSink(State, C.getPredecessor());
+ return;
----------------
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?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:19
+ProgramStateRef relateSymbols(ProgramStateRef State, SymbolRef Sym1,
+ SymbolRef Sym2, bool Equal);
----------------
Docs, docs, docs.
================
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>()) {
----------------
Perhaps we should guard `Op` with an assert, I mean it should be either == or
!=.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:276
+ if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
+ if ((State = relateSymbols(State, Sym1, Sym2,
+ (Op == OO_EqualEqual) ==
----------------
Could we spare the `if` with just keeping the declaration of `State` and then
```
return std::make_pair(State, nullptr);
```
?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:277-278
+ if ((State = relateSymbols(State, Sym1, Sym2,
+ (Op == OO_EqualEqual) ==
+ (TruthVal->getValue() != 0)))) {
+ return std::make_pair(State, nullptr);
----------------
This is hard to read for me. Also, Isn't it enough just to write
`TruthVal->getValue()` ?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:290
+ if (StateTrue)
+ StateTrue = StateTrue->assume(RetVal, true);
+ if (StateFalse)
----------------
Why do we `assume` here again? We already had an assume in `relateSymbols`,
hadn't we?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:301
+
+ // FIXME: This code should be reworked as follows:
+ // 1. Subtract the operands using evalBinOp().
----------------
Please explain in the comment what is the problem with the current impl.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:313-314
+
+ auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal);
+ if (!NewState)
+ return nullptr;
----------------
Could we merge the declaration of the new state into the parens of the `if`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.h:172
long Scale);
+std::pair<ProgramStateRef, ProgramStateRef>
+assumeComparison(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2,
----------------
Documentation would be helpful.
================
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)
----------------
It seems like we don't use it anywhere.
Repository:
rG LLVM Github Monorepo
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
