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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!

I don't have great context on tidy, so I can't stamp this, but I do have a few drive-by nits for you.



================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:22
+                       functionDecl(returns(isInteger()), hasName("pipe"),
+                                    //hasParameter(0, hasType(constantArrayType(hasElementType(isInteger()), hasSize(2)))))
+                                    hasParameter(0, hasType(pointsTo(isInteger())))));
----------------
We probably don't want to commit commented out code


================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:27
+void CloexecPipeCheck::check(const MatchFinder::MatchResult &Result) {
+  const std::string &ReplacementText =
+      (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();
----------------
simplicity nit: can this be a `std::string`?


================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h:18
+
+/// accept() is better to be replaced by accept4().
+///
----------------
nit: should probably update this comment


================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:9
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC [android-cloexec-pipe]
+  // CHECK-FIXES: pipe2(pipefd, O_CLOEXEC);
+}
----------------
(Do we have a CHECK-FIXES-NOT or CHECK-MESSAGES-NOT to apply to the things below? Or are the CHECKs here meant to be complete like clang's `-verify`?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61967