[PATCH] D71510: [clang][checkers] Added new checker 'error-return-checker'. (WIP)

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

Great job, this seems to be progressing nicely! please see my comments inline.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:38
+  // See if the result value from the system function (to check) is checked for
+  // error after a branch condition. 'Value' contains the (mostly conjured)
+  // symbolic value of the function call. 'RetTy' is the return type of the
----------------
Value is not the name of the parameter, maybe a refactoring missed this comment.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:77
+// This should work with any type of null value.
+// FIXME: Is this different from the ValueErrorResultChecker with 0 as value?
+class NullErrorResultChecker : public CheckForErrorResultChecker {
----------------
I think the current implementation is slightly different. The ValueErrorResultChecker uses symbolic evaluation of the equation with a ConcreteInt 0 value, while the ProgramState::isNull checks if the value is a constant and if it is not a zero constant (also calls further to ConstraintManager::isNull).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:158
+// value < 0" check too.
+class NegativeOrEofErrorResultChecker : public CheckForErrorResultChecker {
+public:
----------------
Maybe only checking for negative value is enough? My gut feeling is that the equality check is redundant. I would argue:
let A be: Value is negative
let B be: Value is equal to -1

since B implies A, (A or B) can be simplified to just A.
This presupposes that every time B can reasoned about A can be too.
This seems to be the case for here as well, as the definiteness of being equal to -1 is stronger than being lower than 0.



================
Comment at: clang/test/Analysis/error-return.c:270
+
+These are OK if not checked:
+
----------------
just for the sake of completeness; mention all the cases where error checking is deemed unnecessary as per ERR-33-EX1

https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors#ERR33-C.Detectandhandlestandardlibraryerrors-Exceptions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71510