[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:25
+      callExpr(
+          callee(functionDecl(allOf(
+              isExpansionInFileMatching(HEADER_FILE), returns(asString("int")),
----------------
`functionDecl()` is implicitly `allOf`.


================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:26
+          callee(functionDecl(allOf(
+              isExpansionInFileMatching(HEADER_FILE), returns(asString("int")),
+              hasParameter(1, hasType(isInteger())),
----------------
`isExpansionInFileMatching(...)` is a dependency on the implementation of the library. The fact that `#include <fcntl.h>` provides this macro doesn't mean the macro is defined in this header (it could be in another transitively included header). Same below.


================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:26
+          callee(functionDecl(allOf(
+              isExpansionInFileMatching(HEADER_FILE), returns(asString("int")),
+              hasParameter(1, hasType(isInteger())),
----------------
alexfh wrote:
> `isExpansionInFileMatching(...)` is a dependency on the implementation of the library. The fact that `#include <fcntl.h>` provides this macro doesn't mean the macro is defined in this header (it could be in another transitively included header). Same below.
`asString("int")` is wasteful, since it needs to allocate the string and print the return type name there. `hasType(isInteger())` is a much better alternative.


================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:28
+              hasParameter(1, hasType(isInteger())),
+              anyOf(matchesName("open"), matchesName("open64")))).bind("funcDecl")))
+          .bind("openFn"),
----------------
`matchesName` is wasteful, if you need just to compare the name. You probably want to use `hasAnyName("open", "open64")`.


================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:90
+    return checkFlag(cast<IntegerLiteral>(Flags));
+  } else if (isa<BinaryOperator>(Flags)) {
+    if (cast<BinaryOperator>(Flags)->getOpcode() ==
----------------
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: test/clang-tidy/android-file-descriptor.cpp:3
+
+#include <fcntl.h>
+
----------------
We don't use actual library headers in the tests. Instead, please add mock declarations of the API you need (open, open64, O_ flags, etc.).


Repository:
  rL LLVM

https://reviews.llvm.org/D33304