[PATCH] D118657: Adds TrustReturnsNonnullChecker

[Artem Dergachev via Phabricator via llvm-commits]

NoQ edited reviewers, added: NoQ; removed: dergachev.a.
NoQ added a comment.

Lovely! I have some nits.

@NoQ is my actual account, sorry for the mess >.<



================
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp:1-2
+//== TrustReturnsNonnullChecker.cpp --------- API nullability modeling -*- C++
+//-*--==//
+//
----------------
This line broke.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp:42
+  /// \returns Whether the method declaration has the attribute returns_nonnull.
+  bool isNonNullPtr(const CallEvent &Call, CheckerContext &C) const {
+    QualType ExprRetType = Call.getResultType();
----------------
Parameter `C` appears to be unused.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp:47
+
+    if (Call.getDecl()->hasAttr<ReturnsNonNullAttr>()) {
+      return true;
----------------
`Call.getDecl()` has to be checked for null. Calls with null decls appear when the function being called is unknown, eg. during calls through unknown/symbolic [[ https://cdecl.org/?q=void+*%28*f%29%28void%29 | pointer-to-function ]]:
```lang=c
void test(void *(*f)(void)) {
  // will probably crash the compiler, worth a test case
  f();
}
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustReturnsNonnullChecker.cpp:48
+    if (Call.getDecl()->hasAttr<ReturnsNonNullAttr>()) {
+      return true;
+    }
----------------
```lang=c++
if (x) {
  return true;
}
return false;
```
can usually be shortened to
```lang=c
return x;
```
.


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

https://reviews.llvm.org/D118657