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

[Jian Cai via Phabricator via cfe-commits]

jcai19 marked an inline comment as done.
jcai19 added inline comments.


================
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);
----------------
gribozavr wrote:
> jcai19 wrote:
> > gribozavr wrote:
> > > 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"
> > It appeared to me that there was no easy way to change the warning message due to the way insertMacroFlag is implemented (called in  CloexecPipe2Check::check(...) to add O_CLOEXEC flag when necessary). The function always issue a warning in the format of "%0 should use %1 where possible". So unless we parameterize the warning string (which could be solved by a different code change), we might end up implementing a similar function with different warning message, which creates extra work if we need to modify insertMacroFlag in the future. I am new to Clang so please let me know if there is a better way to udpate the warning message.
> I don't see an issue with parameterizing insertMacroFlag -- the current message that it produces is not descriptive enough anyway, users tend to ignore such messages.
I agree. But my point is insertMacroFlag is used by several other checks. Shouldn't we parameterize the warning message and update these checks together in a separate change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62049