Page MenuHomePhabricator

[clang-tidy] Add a new Android check "android-cloexec-socket"
ClosedPublic

Authored by yawanng on Jun 30 2017, 3:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

yawanng created this revision.Jun 30 2017, 3:21 PM
chh accepted this revision.Jun 30 2017, 4:54 PM

LGTM. If nobody needs this during the long weekend, it's better to submit after the long weekend.

This revision is now accepted and ready to land.Jun 30 2017, 4:54 PM
Eugene.Zelenko requested changes to this revision.Jun 30 2017, 6:18 PM

Please wait for Alexander, Aaron or Haojian approval.

clang-tidy/android/CloexecOpenCheck.cpp
22 ↗(On Diff #104946)

Anonymous namespaces should contain only class declarations. static is enough.

clang-tidy/android/CloexecSocketCheck.cpp
21 ↗(On Diff #104946)

Anonymous namespaces should contain only class declarations. static is enough.

docs/ReleaseNotes.rst
79 ↗(On Diff #104946)

Maybe present in arguments of instead of exist will sound better?

docs/clang-tidy/checks/android-cloexec-socket.rst
5 ↗(On Diff #104946)

Please add empty line there.

This revision now requires changes to proceed.Jun 30 2017, 6:18 PM
hokein added inline comments.Jul 4 2017, 11:22 AM
clang-tidy/utils/CloexecFlagChecker.h
21 ↗(On Diff #104946)

Maybe put this function to utils/ASTUtils.h file.

The name CloseOnExecFlag is too specific, it actually checks whether the flags expr has a given flag IMO, might be call it hasFlag (a better name are welcome), and document more details of the API in the above comment.

test/clang-tidy/android-cloexec-socket.cpp
20 ↗(On Diff #104946)

I'd use the complete statement for checking the fixes here, the same below.

92 ↗(On Diff #104946)

nit: remove the empty line.

alexfh requested changes to this revision.Jul 5 2017, 6:30 AM
alexfh added inline comments.
test/clang-tidy/android-cloexec-socket.cpp
20 ↗(On Diff #104946)

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.

yawanng updated this revision to Diff 105293.Jul 5 2017, 10:03 AM
yawanng edited edge metadata.
yawanng marked 8 inline comments as done.
alexfh requested changes to this revision.Jul 10 2017, 2:35 AM
alexfh added inline comments.
clang-tidy/utils/ASTUtils.h
24 ↗(On Diff #105293)

s/presents/is present/

27 ↗(On Diff #105293)

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.

28 ↗(On Diff #105293)

CloseOnExecFlag is too specific for a function that is meant to be somewhat generic.

docs/ReleaseNotes.rst
79 ↗(On Diff #105293)

s/presents/is present/

test/clang-tidy/android-cloexec-socket.cpp
20 ↗(On Diff #104946)

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.

65 ↗(On Diff #105293)

// CHECK-MESSAGES-NOT: warning: is redundant. The test script runs FileCheck with -implicit-check-not={{warning:|error:}}, which takes care of stray warning: lines.

This revision now requires changes to proceed.Jul 10 2017, 2:35 AM
yawanng updated this revision to Diff 105878.Jul 10 2017, 10:42 AM
yawanng edited edge metadata.
yawanng marked 5 inline comments as done.
yawanng added inline comments.
clang-tidy/utils/ASTUtils.h
27 ↗(On Diff #105293)

Yes, the name is a little misleading and only consider the macro is not thorough. But for the purpose of the current Android checks, where this function is used, this simple solution can cover about 95% cases. Few use enum or constexpr for flags like O_CLOEXEC or other similar ones. Moreover, if extending the function to cover the constant integer variables, it's not easy to get the value of the checked flag, which may depend on the target. Generally, in current stage, this function works well in the Android cases and it's still a TODO for other more sophisticated user cases.

alexfh requested changes to this revision.Jul 11 2017, 8:33 AM
alexfh added inline comments.
clang-tidy/utils/ASTUtils.h
24 ↗(On Diff #105878)

nit: Third-person singular form is better for interface comments ("<the function> Checks whether a macro flag .... <the function> Only considers cases of single match ....")

28 ↗(On Diff #105878)

StringRef is a better type for read-only string arguments.

28 ↗(On Diff #105878)

FlagSpelling or FlagName could be better than just Flag.

docs/clang-tidy/checks/android-cloexec-socket.rst
6 ↗(On Diff #105878)

A bit more expanded explanation on where the descriptor can leak would make the documentation much better. If there's a good external information source describing this issue, you can also link there.

This revision now requires changes to proceed.Jul 11 2017, 8:33 AM
yawanng updated this revision to Diff 106058.Jul 11 2017, 9:59 AM
yawanng edited edge metadata.
yawanng marked 4 inline comments as done.

Please wait for Alexander, Aaron or Haojian approval.

Could you please check this CL? Thank you.

Please wait for Alexander, Aaron or Haojian approval.

Could you please check this CL? Thank you.

You could commit changes, since Alexander approved them.

This revision was automatically updated to reflect the committed changes.