Charusso added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the target triple. ---------------- Szelethus wrote: > This isn't true: the user may decide to only enable non-pathsensitive > checkers. > > I think the comment should rather state that these whether these checkers are > enabled shouldn't be explicitly specified, unless in **extreme** > circumstances (causes a crash in a release build?). Well, I have removed it instead. Makes no sense, you are right. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145 + + // Get the concrete return value. + Optional<const bool *> RawConcreteValue = CDM.lookup(*Call); ---------------- NoQ wrote: > 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. Great advice, thanks you! 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