[PATCH] D34913: [clang-tidy] Add a new Android check "android-cloexec-socket"

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/utils/ASTUtils.h:24
+
+/// Check whether a macro flag presents in the given argument. Only consider
+/// cases of single match or match in a binary OR expression. For example,
----------------
s/presents/is present/


================
Comment at: clang-tidy/utils/ASTUtils.h:27
+/// <needed-flag> or <flag> | <needed-flag> | ...
+bool hasFlag(const Expr *Flags, const SourceManager &SM,
+             const LangOptions &LangOpts, const char *CloseOnExecFlag);
----------------
`hasFlag` is too generic and may give a totally wrong impression of what the function does. Even in the context of this utility library I would prefer to expand the name and make it less ambiguous. Something along the lines of `exprHasBitFlagWithSpelling`. I wouldn't also insist on the flag being implemented as a macro, since this restriction would prevent many valid use cases with enum or constexpr int flags.


================
Comment at: clang-tidy/utils/ASTUtils.h:28
+bool hasFlag(const Expr *Flags, const SourceManager &SM,
+             const LangOptions &LangOpts, const char *CloseOnExecFlag);
 } // namespace utils
----------------
`CloseOnExecFlag` is too specific for a function that is meant to be somewhat generic.


================
Comment at: docs/ReleaseNotes.rst:79
+
+  Checks if the required file flag ``SOCK_CLOEXEC`` presents in the argument of
+  ``socket()``.
----------------
s/presents/is present/


================
Comment at: test/clang-tidy/android-cloexec-socket.cpp:20
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket]
+  // CHECK-FIXES: SOCK_STREAM | SOCK_CLOEXEC
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM, 0));
----------------
alexfh wrote:
> hokein wrote:
> > I'd use the complete statement for checking the fixes here, the same below.
> Yes, CHECK-FIXES has fewer context than CHECK-MESSAGES (it doesn't have the line number, for example), and there's much more stuff that can go wrong than with the static diagnostic messages. So please make CHECK-FIXES as specific, as possible. Ideally, they should be unique, so that a wrong line can't be matched by a CHECK-FIXES. If needed, add unique comments on identical lines.
This is not addressed yet. The test contains a number of ambiguous CHECK-FIXES patterns. Please make each pattern (and the line being matched) unique to avoid incorrect matches. One way to do this is to add unique trailing comments to each of the similar lines and include this comment into the pattern.


================
Comment at: test/clang-tidy/android-cloexec-socket.cpp:65
+  socket(0, SOCK_CLOEXEC, 0);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(socket(0, SOCK_CLOEXEC, 0));
----------------
`// CHECK-MESSAGES-NOT: warning:` is redundant. The test script runs FileCheck with `-implicit-check-not={{warning:|error:}}`, which takes care of stray `warning:` lines.


https://reviews.llvm.org/D34913