[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:63
+                                          const BinaryOperator *BinOp,
+                                          const llvm::APSInt *KnownValue,
+                                          QualType RetTy,
----------------
Szelethus wrote:
> This is the value we're checking //against//, I think the name `KnownValue` doesn't convey this.
Renamed to `ValueToTestAgainst`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:79-80
+                                  bool ChildIsLHS) const override {
+    if (!KnownValue)
+      return false;
+
----------------
balazske wrote:
> Szelethus wrote:
> > So if we failed to get retrieve the value we're checking against, we automatically assume that its not a proper check? Shouldn't we be conservative here? What if that value is something like `getEOFValue()`?
> This case needs attention, now it is detected as failure (maybe the function can return `Optional<bool>` to indicate a non-determinable case).
This case is now not error.
A test is added for this case.
```
  int R = fputs("str", file());
  if (R == getEOFValue())
    return;
```
This is no warning now (but `getEOFValue` can return anything).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:83
+    bool KnownNull = KnownValue->isNullValue();
+    bool KnownEOF = ((*KnownValue) == BVF.getValue(-1, RetTy));
+
----------------
Szelethus wrote:
> We have a smarter EOF getter thanks to @martong, don't we?
`tryExpandAsInteger` is used now.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:220-223
+/// Information about a specific function call that has an error return code to
+/// check. This data is stored in a map and indexed by the SymbolRef that stands
+/// for the result of the function call.
+struct CalledFunctionData {
----------------
Szelethus wrote:
> The comments leave me to believe that the correct class name would be `FunctionCallData`, right? Because `FnInfo` does the job of describing the called function, and this one deals with the actual call.
`CalledFunctionData` was renamed to `FunctionCallData` and `CFD` variables to `Call`. It looks better now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72705