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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

just a few drive-by nits/comments from me. as usual, not super familiar with clang-tidy, so i won't be able to stamp this.

thanks!



================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:23
+      binaryOperator(
+          hasOperatorName("<"),
+          hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), unless(hasName("posix_openpt")))))),
----------------
should we also try to catch `<= ${negative_value}` (or `< ${negative_value}`)?


================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:29
+      binaryOperator(
+          hasOperatorName("=="),
+          hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), unless(hasName("posix_openpt")))))),
----------------
similarly, should we complain about `!= ${negative_value}`?


================
Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:36
+
+
+void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
----------------
nit: please clang-format


================
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");
+}
----------------
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`


================
Comment at: clang-tools-extra/test/clang-tidy/android-posix-return.cpp:57
+
+void WarningWithMacro () {
+  if (posix_fadvise(0, 0, 0, 0) < ZERO) {}
----------------
nit: no space in `Macro ()`, please


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63623