[PATCH] D90691: [analyzer] Add new checker for unchecked return value.

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:41
+                        BugReporter &BR) const {
+    auto FoundCall = callExpr().bind("call");
+    auto CallInCompound = compoundStmt(forEach(FoundCall));
----------------
Please note that the `CallExpr` does not necessarily stands alone. It may be wrapped into an `ExprWithCleanUps`. We should consider these `CallExpr`s as unchecked too.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp:85
+private:
+  llvm::StringMap<int> FunctionsToCheck = {
+      {"aligned_alloc", 2}, {"asctime_s", 3},     {"at_quick_exit", 1},
----------------
Hmm, why `StringMap<>`? Why not `CallDescriptionMap<>`?


================
Comment at: clang/test/Analysis/unchecked-return-value.cpp:10
+int f1(int X) {
+  scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  std::scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
----------------
Please use some valid format here. E.g. `scanf("%*c");`


================
Comment at: clang/test/Analysis/unchecked-return-value.cpp:16
+    scanf(""); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
----------------
Please add such simple test case for all the functions we try to check. (One call is enough for every such function, either in `std::` or on the top namespace.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90691