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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr requested changes to this revision.
gribozavr added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst:6
+
+Checks if the required file flag ``O_CLOEXEC`` is present in the argument of
+``pipe2()``. ``pipe2()`` should include ``O_CLOEXEC`` in its type argument to
----------------
This checks ensures that pipe2() is called with the O_CLOEXEC flag.  This flag helps to avoid accidentally leaking file descriptors to child processes.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst:17
+
+  // becomes
+
----------------
"Suggested replacement"

Also use a separate code block for the replacement.


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:48
+  pipe2(pipefd, O_NONBLOCK);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));
----------------
srhines wrote:
> Much like line 39 (which covers both lines 37 and 38), you can delete this CHECK-MESSAGES-NOT. The one on line 50 will cover both of these.
Not done -- you don't need any CHECK-MESSAGES-NOT at all.


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:20
+  pipe2(pipefd, O_NONBLOCK);
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'pipe2' should use O_CLOEXEC where possible [android-cloexec-pipe2]
+  // CHECK-FIXES: pipe2(pipefd, O_NONBLOCK | O_CLOEXEC);
----------------
Same comment about the message as in D61967 -- the message should briefly explain why the user should make this change. Users tend to ignore warnings they don't understand.

"pipe2 should be called with O_CLOEXEC to avoid leaking file descriptors to child processes"


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:27
+
+void f() {
+  int pipefd[2];
----------------
Please give the tests informative names instead of `a`, `f`, `g` etc.


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:54
+
+void e() {
+  int pipefd[2];
----------------
How is `e` different from `a`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62049