NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:107 + + // If the invariant is broken we do not return the concrete value. + if (IsInvariantBreak) ---------------- Something's wrong with formatting. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:109 + if (IsInvariantBreak) + Out << " should return "; + else ---------------- I'd still like to avoid "should". We are in no position to teach them what should their method return. It's not a matter of convention; it's a matter of correctness. We don't want people to go fix their code to return true when they see a note. We only need to point out that the return value is not what they expect. I suggest removing this note entirely, i.e., early-return when we see an invariant break in `checkPostCall`, because we've already emitted a note in `checkEndFunction`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145 + + // Get the concrete return value. + Optional<const bool *> RawConcreteValue = CDM.lookup(*Call); ---------------- NoQ wrote: > I think you can squeeze all this code into one big `isInvariantBreak(Call, > ReturnV)` and re-use it across both methods. I think it would be more precise to talk about "expected"/"actual" return value. The actual return value may be either concrete (i.e., `nonloc::ConcreteInt`) or symbolic (i.e., `nonloc::SymbolVal`). Also, there's a relatively famous rule of a thumb for making good comments: "comments shouldn't explain what does the code do, but rather why does it do it". Instead of these comments i'd rather have one large comment at the beginning of `checkEndFunction` explaining what are we trying to do here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145-157 + // Get the concrete return value. + Optional<const bool *> RawConcreteValue = CDM.lookup(*Call); + if (!RawConcreteValue) + return; + + // Get the symbolic return value. + SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext()); ---------------- I think you can squeeze all this code into one big `isInvariantBreak(Call, ReturnV)` and re-use it across both methods. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:170 + + // The following is swapped because the invariant is broken. + Out << '\'' << Name << "' returns " ---------------- Something's wrong with formatting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
