pipe2() is better to set O_CLOEXEC flag to avoid file descriptor leakage.
I'm not all that familiar with writing clang-tidy-specific tests, but should these tests here denote that a diagnostic should NOT be issued? That is usually the convention in regular Clang tests, so I assume the test runner here should be equally supportive of ensuring that the contents passed through without any other diagnostics related to pipe2 and/or O_CLOEXEC.
That makes sense, and I have seem tests for similar checks with (e.g. android-cloexec-open) and without (e.g. android-cloexec-accep4 and android-cloexec-socket) additional CHECK-MESSAGES-NOT check. But based on the Testing Checks section of https://clang.llvm.org/extra/clang-tidy/Contributing.html, it seems typically CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. Please let me know what you think.
If you look in test/clang-tidy/android-cloexec-creat.cpp, you will see that there are "CHECK-MESSAGES-NOT" checks that ensure the diagnostic is not issued in correct cases. You can put the checks on lines 39, 58, and 67, which will ensure that there are no additional diagnostics being generated.
Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.
This checks ensures that pipe2() is called with the O_CLOEXEC flag. This flag helps to avoid accidentally leaking file descriptors to child processes.
Also use a separate code block for the replacement.
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"
Please give the tests informative names instead of a, f, g etc.
Not done -- you don't need any CHECK-MESSAGES-NOT at all.
How is e different from a?
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.
warning1, warning2 is better than a and b, but we can do even better.
Similarly in the rest of the patch.
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?
Sounds good. I have updated the function names, although I feel the "ForPipeCall" part is already implied in the test so I did not include it to keep the names shorter. Let me know if you think it is better to have it included.