[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

[Jian Cai via Phabricator via cfe-commits]

jcai19 marked 10 inline comments as done.
jcai19 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:23
+      binaryOperator(
+          hasOperatorName("<"),
+          hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), unless(hasName("posix_openpt")))))),
----------------
george.burgess.iv wrote:
> should we also try to catch `<= ${negative_value}` (or `< ${negative_value}`)?
Sounds good. Will update the patch to catch these two cases as well.


================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:29
+      binaryOperator(
+          hasOperatorName("=="),
+          hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), unless(hasName("posix_openpt")))))),
----------------
george.burgess.iv wrote:
> similarly, should we complain about `!= ${negative_value}`?
Sounds good. Will update the patch to catch these additional cases as well.


================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:39
+  const auto &BinOp = *Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  diag(BinOp.getOperatorLoc(), "posix functions (except posix_openpt) never return negative values");
+}
----------------
george.burgess.iv wrote:
> would it be helpful to add fixits for simple cases? e.g. we can probably offer to replace `posix_whatever() < 0` with `posix_whatever() > 0` or `!= 0`
While this fix is handy, I am not sure whether it will be safe enough under all circumstances. For example, is it possible in the code block following the check, the program calls another POSIX function and alter the errno before its value it checked? In that case, maybe the proper fix should be something as follows and fixing it by changing the binary operator may obscure it:

int ret = posix_whatever();
if (ret != 0)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63623