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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
