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

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:55-60
+  /// Test if an encountered binary operator where the return value is involved
+  /// is a valid check statement. The return value appears in one side of the
+  /// operator (`ChildIsLHS` indicates if it is on the LHS). If the other side
+  /// contains a known (mostly constant) value, it is already calculated in
+  /// `KnownValue`. `RetTy` is the type of the return value (return type of the
+  /// function call in the code to check).
----------------
Szelethus wrote:
> So, for the time being we're saying that a checking must involve a binary operator. That sounds about right, but I wonder if there is a case we're not thinking about.
For the "fputs" case yes, comparison to EOF (or negative) can be detected by binary operator. The extended version has support for unary operator too (and some other tricks) (detect check of nullness of a value).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:79-80
+                                  bool ChildIsLHS) const override {
+    if (!KnownValue)
+      return false;
+
----------------
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).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:329-330
+  // Value appears in other statement.
+  // FIXME: Statements that affects control-flow should be checked separately.
+  // For example `Child` may appear as a condition of `if`.
+  VisitResult VisitStmt(const Stmt *S) { return StopExamineNoError; }
----------------
Szelethus wrote:
> Why?
This comment looks invalid now, can be removed. (`VisitIfStmt` already handles the case.)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:428-432
+  // Check for explicit cast to void.
+  if (auto *Cast = dyn_cast<const CStyleCastExpr>(S)) {
+    if (Cast->getTypeAsWritten().getTypePtr()->isVoidType())
+      return nullptr;
+  }
----------------
Szelethus wrote:
> Ah, okay, so you mean to check whether someone did something like this:
> ```lang=c++
> // Silence compiler warning.
> (void)localtime(...);
> ```
> 
> I don't think that the parent though will get you the correct answer, how about this: 
> 
> ```lang=c++
> // Silence compiler warning.
> (void) (coin() ? NULL : localtime(...));
> ```
> 
> Please put a TODO here.
Probably this case can be handled by the `ErrorCheckTestStmtVisitor` in better way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72705