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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:28
+                                   hasParameter(1, hasType(isInteger())),
+                                   hasAnyName("open", "open64"))
+                          .bind("funcDecl")))
----------------
I'd put the `hasAnyName` matcher in front of `hasParameter` which follows the order of function declaration.

The same below.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:46
+  const Expr *FlagArg;
+  if ((MatchedCall = Result.Nodes.getNodeAs<CallExpr>("openFn")))
+    FlagArg = MatchedCall->getArg(1);
----------------
How about 

```
const Expr *FlagArg = nullptr;
if (const auto* OpenFnCall = Result.Nodes.getXXX()) {
  // ...
} else if (const auto* OpenAtFnCall = Result.Nodes.getXX()) {
  // ...
}
assert(FlagArg);
```
? 


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61
+  SourceLocation EndLoc =
+      Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts());
+
----------------
Instead of using getLangOpts(), you should use `Result.Context.getLangOpts()`.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:67
+
+bool FileOpenFlagCheck::checkFlags(const Expr *Flags, const SourceManager &SM) {
+  bool IsFlagIn;
----------------
No need to be a class member method, put it inside this translation unit. 


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79
+    std::pair<FileID, unsigned> ExpansionInfo = SM.getDecomposedLoc(Loc);
+    auto MacroName =
+        SM.getBufferData(ExpansionInfo.first)
----------------
You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the marco name, or doesn't it work here?


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:84
+
+    IsFlagIn = (MacroName == "O_CLOEXEC");
+
----------------
I think you can use early return here. With that you don't need to maintain the flag variable `IsFlagIn`, so the code structure like:

```
if (isa<..>) {
   
    return ...;
}

if (isa<BinaryOperator>()) {
    // ...
    return ...;
}
retrun false;
```


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:88
+  // If it's a binary OR operation.
+  else if ((isa<BinaryOperator>(Flags)) &&
+           (cast<BinaryOperator>(Flags)->getOpcode() ==
----------------
You can use `if (const auto* BO = dyn_cast<BinaryOperator>(Flags))`, so that you don't call `cast<BinaryOperator>(Flags)` multiple times below.


================
Comment at: test/clang-tidy/android-file-open-flag.cpp:76
+void e() {
+  open("filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
----------------
I would add tests where `O_CLOEXEC` is not at the end of parameter 2 expression like `open("filename", O_RDWR |  O_CLOEXEC | O_EXCL)`.


https://reviews.llvm.org/D33304