[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[Jian Cai via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:31
+      Result,
+      "prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC",
+      ReplacementText);
----------------
gribozavr wrote:
> hokein wrote:
> > the message doesn't seem to explain the reason, especially to the people who are not familiar with the `pipe` API, maybe `prefer pipe2() to pipe() to avoid file descriptor leakage` is clearer?
> > 
> > Ah, it looks like you are following the existing `CloexecCreatCheck` check, no need to do it in this patch. 
> "prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes"
> 
> No need to change in this patch if you're following an existing pattern, but please do update the message in all checks in a separate patch to explain things better.
Thanks for the comments. I have updated the text. 


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:17
+  pipe(pipefd);
+  // CHECK-MESSAGES-NOT: warning:
+}
----------------
srhines wrote:
> hokein wrote:
> > nit: no need to do it explicitly, if a warning is shown unexpectedly, the test will fail.
> I somehow never realized this (and there seem to be several places that also don't realize it). Thanks for letting us know.
Thanks for the clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61967