This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][Part3] Add a new module Android and three new checks.
ClosedPublic

Authored by yawanng on May 31 2017, 4:24 PM.

Diff Detail

Event Timeline

yawanng created this revision.May 31 2017, 4:24 PM
yawanng edited the summary of this revision. (Show Details)Jun 1 2017, 3:34 PM
yawanng updated this revision to Diff 101129.Jun 1 2017, 3:38 PM

Modify the format.

hokein added inline comments.Jun 2 2017, 4:14 AM
clang-tidy/android/FopenModeCheck.h
20

Is there a guideline/code style describing it? If so, would be nice to add the link.

And I'd suggest you to put the detailed document to the rst instead of code comments -- because users usually read documents.

27

nit: remove the blank line.

34

No need to put utility functions/variables inside the class, you can define them in an anonymous namespace in the implementation file, the same below.

docs/clang-tidy/checks/android-fopen-mode.rst
5

nit: remove extra trailing ===

test/clang-tidy/android-fopen-mode.cpp
14

Use completed statements fopen("filename", "re"); to do the fix verification. The same below.

17

You can use a shorter string here -- It is sufficient as we have a completed message check in the first CHECK-MESSAGES.

yawanng updated this revision to Diff 101251.Jun 2 2017, 11:31 AM
yawanng marked 4 inline comments as done.
yawanng added inline comments.
clang-tidy/android/FopenModeCheck.h
20

Adding 'e' is like adding the "O_CLOEXEC". The reason is the same with part1(https://reviews.llvm.org/D33304). Added more text in doc.

34

Inline one and move the other and the variable to anonymous namespace.

test/clang-tidy/android-fopen-mode.cpp
14

The fix will mark the corresponding argument and suggest the fix like:

fopen("filename", "r");

^~
 re

Is this clear to show the fix?

yawanng updated this revision to Diff 101270.Jun 2 2017, 1:43 PM
hokein added inline comments.Jun 7 2017, 9:37 AM
clang-tidy/android/FopenModeCheck.cpp
32

We may not need this utility function, see my comment on your part 2.

58

How about if (const auto* ModeStr = dyn_cast<StringLiteral>(...))?

70

remove the trailing ., clang-tidy message is not a sentence.

And we can simplify the code like: diag(...) << FD << FixItHint::CreateReplacement(ModeRange.getSourceRange(), ReplacementText);

test/clang-tidy/android-fopen-mode.cpp
14

CHECK-FIXES is not used to check the message of the fix. It is used to check the code after the generated-fix is applied. So here should be // CHECK-FIXES: fopen("filename", "re");.

yawanng updated this revision to Diff 101830.Jun 7 2017, 3:25 PM
yawanng marked 4 inline comments as done.
yawanng updated this revision to Diff 103795.Jun 23 2017, 4:20 PM

url format fix in doc.rst

hokein added inline comments.Jun 26 2017, 8:42 AM
clang-tidy/android/FopenModeCheck.cpp
31

You can use Twine here which is more efficient: (getSourceText(...) + " \"" + Twine(Mode) + "\"").str().

The same below.

65

We'd prefer early return in llvm, and with that the flag "MayHaveE" can be removed.

76

I think you can use ModeArg->getSourceRange().

Moreover, as ModeRange is used only once, you can ModeArg->getSourceRange() directly in FixtItHint below.

yawanng updated this revision to Diff 104004.Jun 26 2017, 12:02 PM
yawanng marked 3 inline comments as done.
hokein accepted this revision.Jun 27 2017, 12:22 PM

One comment on test, otherwise looks good.

test/clang-tidy/android-fopen-mode.cpp
29

What if str = "re" here? Seems to me the check will omit this case. Would be nice to add this test case.

This revision is now accepted and ready to land.Jun 27 2017, 12:22 PM
yawanng updated this revision to Diff 104269.Jun 27 2017, 2:03 PM
yawanng marked an inline comment as done.
yawanng updated this revision to Diff 104570.Jun 28 2017, 6:14 PM

rename the check.

chh edited edge metadata.Jun 29 2017, 10:25 AM

Summary should be updated. s/android-fopen-mode/android-cloexec-fopen/

yawanng closed this revision.Jun 29 2017, 10:42 AM