[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

The code looks good to me now, I'd wait to see whether @alexfh has further comments before submitting the patch. Thanks for your patience with the review ;)



================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61
+  SourceLocation EndLoc =
+      Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts());
+
----------------
yawanng wrote:
> hokein wrote:
> > Instead of using getLangOpts(), you should use `Result.Context.getLangOpts()`.
> May I ask what's the difference between the `Result.Context.getLangOpts()` and `getLangOpts()`? Does `Result.Context.getLangOpts()` have a longer lifetime than `getLangOpts()`?
IMO they are the same fundamentally, except `Result.Context.getLangOpts()` can save a copy. We usually use `Result.Context.getLangOpts()` inside `ClangTidyCheck::check(const MatchFinder::MatchResult &Result)`.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79
+    std::pair<FileID, unsigned> ExpansionInfo = SM.getDecomposedLoc(Loc);
+    auto MacroName =
+        SM.getBufferData(ExpansionInfo.first)
----------------
yawanng wrote:
> hokein wrote:
> > You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the marco name, or doesn't it work here?
> `getImmediateMacroName` sometimes cannot return the `O_CLOEXEC `, like
> 
> #define O_CLOEXEC  _O_CLOEXEC
> #define _O_CLOEXEC  1
> 
> `getImmediateMacroName` will return `_O_CLOEXEC`, whereas `O_CLOEXEC` is what we need.  I change this to directly get the source text if it's a macro, which seems to be simpler.
Ah, I see, thanks for the clarification!


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:22
+namespace {
+bool checkFlags(const Expr *Flags, const SourceManager &SM,
+                const LangOptions &LangOpts) {
----------------
`checkFlags` name is a bit of ambiguous, maybe rename it to "HasCloseOnExecFlag"?

Notice that you use the string "O_CLOEXEC" several times in the code,  I'd suggest to use a "CloseOnExecFlag" variable. 


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:33
+
+    return (MacroName == "O_CLOEXEC");
+  }
----------------
Nit: The outermost () is not needed.


https://reviews.llvm.org/D33304