[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I'm not sure this approach is the best to deal with the boilerplate, but it seems like an improvement compared to repeating code. See a few comments inline.



================
Comment at: clang-tidy/android/CloexecCheck.cpp:66
+
+StringRef CloexecCheck::getSpellingArg(const MatchFinder::MatchResult &Result,
+                                       int N) const {
----------------
1. Should this be a class method?
2. Is this ever used? (if it is going to be used in a different check, let's postpone its addition to that moment)


================
Comment at: clang-tidy/android/CloexecCheck.h:43
+        ast_matchers::callExpr(
+            ast_matchers::callee(FuncDecl().bind(FuncDeclBindingStr)))
+            .bind(FuncBindingStr),
----------------
Can we make this method non-template and put its implementation to the .cpp file? It could accept a Matcher<FunctionDecl> or something like that and just apply it:

void registerForCall(MatchFinder *Finder, Matcher<FunctionDecl> Function) {
  Finder->addMatcher(calExpr(callee(functionDecl(isExternC(), Function).bind(...))).bind(...), this);
}

Then you could make `FuncDeclBindingStr` and `FuncBindingStr` local to the file.


================
Comment at: clang-tidy/android/CloexecCheck.h:47
+  }
+  /// Currently, we has three types of fixes.
+  ///
----------------
s/we has/we have/


================
Comment at: clang-tidy/android/CloexecCheck.h:102-137
+  ast_matchers::internal::Matcher<FunctionDecl> hasIntegerParameter(int N) {
+    return ast_matchers::hasParameter(
+        N, ast_matchers::hasType(ast_matchers::isInteger()));
+  }
+
+  ast_matchers::internal::Matcher<FunctionDecl>
+  hasCharPointerTypeParameter(int N) {
----------------
These methods don't add much value: `hasParameter(N, hasType(isInteger()))` is not much longer or harder to read than `hasIntegerParameter(N)`, etc. Also having these utilities as non-static methods doesn't make much sense. If you want a shorter way to describe function signatures, I would suggest a single utility function `hasParameters()` that would take a list of type matchers and apply them to the corresponding parameters. That one could be useful more broadly than in this specific check, so it could be placed in utils/ (and later in ASTMatchers.h, if we find enough potential users).


================
Comment at: clang-tidy/android/CloexecCheck.h:140
+private:
+  const static char *FuncDeclBindingStr;
+  const static char *FuncBindingStr;
----------------
You're missing one more const: `static const char * const FuncDeclBindingStr;`


https://reviews.llvm.org/D35372