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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
