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