Page MenuHomePhabricator

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

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
19 ↗(On Diff #101129)

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.

26 ↗(On Diff #101129)

nit: remove the blank line.

33 ↗(On Diff #101129)

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.

4 ↗(On Diff #101129)

nit: remove extra trailing ===

13 ↗(On Diff #101129)

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

16 ↗(On Diff #101129)

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.
19 ↗(On Diff #101129)

Adding 'e' is like adding the "O_CLOEXEC". The reason is the same with part1( Added more text in doc.

33 ↗(On Diff #101129)

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

13 ↗(On Diff #101129)

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

fopen("filename", "r");


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
57 ↗(On Diff #101251)

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

69 ↗(On Diff #101251)

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

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

31 ↗(On Diff #101270)

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

13 ↗(On Diff #101129)

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
64 ↗(On Diff #101830)

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

75 ↗(On Diff #101830)

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

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

30 ↗(On Diff #103795)

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

The same 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.

28 ↗(On Diff #104004)

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