gribozavr 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);
----------------
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.
================
Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:17
+
+void warning1() {
+ int pipefd[2];
----------------
`warning1`, `warning2` is better than `a` and `b`, but we can do even better.
`warningForPipeCall`
`warningForPipeCallInMacroArgument`
Similarly in the rest of the patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62049/new/
https://reviews.llvm.org/D62049
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
