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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
