[PATCH] D63623: [clang-tidy] new check: bugprone-posix-return

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp:49
+    diag(OperatorLoc, "POSIX functions (except posix_openpt) never return negative values")
+        << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str());
+    return;
----------------
jcai19 wrote:
> gribozavr wrote:
> > Please add tests for fix-its.
> The fix-it is triggered when cases like posix_*(...) < 0 are detected (except posix_openpt) , which are currently covered in the unit test. The test runtime will make sure fix-its are applied.
Could you add CHECK-FIXES lines to validate the fixit directly?


================
Comment at: clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp:96
+  if (posix_fadvise(0, 0, 0, 0) < 0) {}
+  if (posix_fadvise(0, 0, 0, 0) >= 0) {} else {}
+  if (posix_fadvise(0, 0, 0, 0) == -1) {}
----------------
jcai19 wrote:
> gribozavr wrote:
> > Shouldn't there be a warning in the else branch?
> This check only matches calls to the POSIX functions in the global scope, not member functions or functions defined within namespaces. Although in the global scope,  this particular case will still pass as there won't be a ``<`` binary operator for the else branch so no matching will happen.
Sorry, yes, that's what I meant -- if we warn on `< 0`, then we should warn on the else branch of `>=`. I just commented on the wrong test -- sorry about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63623