gribozavr marked an inline comment as done. 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: > > 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? SGTM! 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
