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) {
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
+  const static char *FuncDeclBindingStr;
+  const static char *FuncBindingStr;
You're missing one more const: `static const char * const FuncDeclBindingStr;`

cfe-commits mailing list

Reply via email to