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

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.


nit: remove the blank line.


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.


nit: remove extra trailing ===


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


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.

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


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


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

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


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


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

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


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

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

The same below.


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


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.


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